Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed self.prior to self.logprior and transformed into PEP8 #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

exowanderer
Copy link

I was trying to run the 3 examples given on the tutorial page; but they would process because self.prior does not exist.

So I edited the code to change self.prior to self.logprior, which does exist. I then realized that UniformPrior and GaussianPrior are actually "log priors" themselves.

While editing the code, to make it readable on my Sublime, I had to adjust for PEP8 compliance -- I use AutoPEP8 with Sublime, so a lot of it was automated.

I hope that this helps. The example now runs; but the results are not the same as on the docs pages. I think that the boundaries are mismatched.

@mirca
Copy link
Member

mirca commented Mar 30, 2020

hey @exowanderer, thanks! however, could you provide the exact error message that you get? on my end the examples run just fine.

@mirca
Copy link
Member

mirca commented Mar 30, 2020

Oh, I see now. The bug is actually in the __repr__ method where self.prior is referenced. So, it looks to me that the best course of action would be to just change self.prior to self.logprior in each __repr__. Thanks for the PEP8 adjustments, but could you do that in a separate PR so that this PR is more concise and easier to review?

@exowanderer
Copy link
Author

Hi Mirca,

I agree that the self.prior -> self.logprior in the __repr__ methods was the global issue. It's actually been so long since I fixed this, that I will have to re-download your version and re-run it. No problem; I'll do it.

As for separating the PEP8 form the __repr__ method call:
I wish that I could say "yes" to separating the PEP8 vs __repr__ method fixes; but my Sublime Text does 90% the PEP8 for me. I could submit it with only the basic formatting. I edited the .format strings to be f-strings, in an effort to read the code after Sublime Text auto-PEP8 formatted it.

Which would you prefer first:
(1) Only edit the self.prior in the necessary __repr__ method (+ auto-PEP8 white space formatting)
(2) Only edit the __repr__ method for f-strings (+ auto-PEP8 white space formatting)

@mirca
Copy link
Member

mirca commented Mar 31, 2020

@exowanderer let's go with (1) first!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants