-
Notifications
You must be signed in to change notification settings - Fork 83
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
feat: Add toy calculator, empirical distribution, and toy example notebook #790
Conversation
This pull request introduces 1 alert when merging 74161b7 into 49ab2a4 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 7425f01 into 49ab2a4 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging b24726f into ed483f4 - view on LGTM.com new alerts:
|
bac9c58
to
5479452
Compare
Codecov Report
@@ Coverage Diff @@
## master #790 +/- ##
==========================================
+ Coverage 97.14% 97.18% +0.04%
==========================================
Files 62 63 +1
Lines 3611 3663 +52
Branches 521 523 +2
==========================================
+ Hits 3508 3560 +52
Misses 64 64
Partials 39 39
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the notebook yet, but things look good here. I've added nitpick comments RE: things rendering our correctly in the docs. The main thing that needs to get added is tests as the coverage on the patch is quite low.
This will be really nice to have in, so thanks for making this.
@kratsg I made the following edits to the notebook and have some questions. I'm happy to discuss or roll back any of them.
@lukasheinrich I think you didn't like this originally though, correct? It seems like this makes it easier for someone to experiment with the notebook.
Shouldn't this be"N=2000" given the numbers that show up in the notebook?
Shouldn't this be "the nominal signal+background model were true"? This is
plt.rcParams.update({"font.size": 14})
fig.set_size_inches(13.5,6)
fig.tight_layout(pad=2.0)
Let me know your thoughts and if you disagree with anything that I can fix up. I think this is a really nice notebook and I'm very excited that we finally have this made now. |
we want to set the uncertainty explicitly. Revert this.
should be 2k.
no.
See chat with Lukas offline.
you lose the square ratio. Should be at least 14,6 if you're doing that.
this is nitpicking for the notebook and should be removed.
nitpicking as well. should be removed.
we don't need tight layout do we? the artists fit fine in the boundary box by default. |
Done
The Skype chat still doesn't make sense. I'll follow up again.
Done. Changed to
The font is pretty small without it. :/
Is the spacing too tight?
I think that these plots can still be confusing (even though there are the nice plot legends) without axis labels. If you don't set padding and apply a vertical-axis label then the right hand plot's label gets scrunched up against the left hand side. The padding solves this. |
I'm realizing that create_calculator shouldn't be exposed by default unless people want to do more advanced things. The interface via |
ok. I guess as long as it's defined at the top of the notebook, then it's fine. |
This is more for me, but this is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukasheinrich @kratsg Unless there is anything else that you want to go in this, I think that once we get examples in the docstrings that this PR can probably get reviewed a final time and go in. That is, unless you want to try to at the end split off the notebook into its own PR.
@lukasheinrich @kratsg I think we can review this now. Also, as ReviewNB wasn't added to the repo before this PR was started here's a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @lukasheinrich for your work on this and a huge amount of patience.
Description
this adds the toy calculator.
Needs: #1147.
ReadTheDocs build:
Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees: