-
Notifications
You must be signed in to change notification settings - Fork 3
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
Norm workflow #361
Norm workflow #361
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
LGTM!
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.
A few questions about the difference between the curves, but the code looks good! Do we think it's worth adding some of the curve info to section 4 in the readme? Most users probably don't need to know the specifics within the notebook, but it is probably good to have an explanation somewhere with a link to the generating_tuning_curve.ipynb
for reference.
Nice! Looks like there was a weird timeout with the coverage check, but other than that it looks good. |
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.
@HPiyadasa, was P9 the curve that looked weird, or M13? We should include the one that looks the most normal.
@HPiyadasa @ngreenwald I got rid of the explicit documentation for selecting 400, 1300, vs 2600 curves, since these curves are specifically for our lab but other users are likely generating their own curves. |
Actually I'll add this documentation back in based on a previous discussion. Per today's discussion any of the norm curves at these settings will work (Mike suggests averaging). |
I agree, let's average them, then name them |
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.
Looks good, just some more description of the new notebook
What is the purpose of this PR?
Closes #325. Closes #344.
How did you implement your changes
See issue template for #344.
Remaining issues
Need to document when to use which tuning curve. Still more tuning curves potentially on the way.