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

Norm workflow #361

Merged
merged 24 commits into from
Jul 3, 2023
Merged

Norm workflow #361

merged 24 commits into from
Jul 3, 2023

Conversation

alex-l-kong
Copy link
Contributor

@alex-l-kong alex-l-kong commented Apr 18, 2023

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.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@alex-l-kong alex-l-kong self-assigned this Apr 18, 2023
@alex-l-kong alex-l-kong marked this pull request as draft April 18, 2023 18:32
@alex-l-kong alex-l-kong marked this pull request as ready for review April 21, 2023 21:55
Copy link
Contributor

@srivarra srivarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@camisowers camisowers left a 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.

src/toffy/normalize.py Outdated Show resolved Hide resolved
@alex-l-kong alex-l-kong requested a review from camisowers April 24, 2023 23:14
@camisowers
Copy link
Contributor

Nice! Looks like there was a weird timeout with the coverage check, but other than that it looks good.

@alex-l-kong alex-l-kong requested a review from ngreenwald April 25, 2023 22:40
Copy link
Member

@ngreenwald ngreenwald left a 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.

@alex-l-kong
Copy link
Contributor Author

@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.

@alex-l-kong
Copy link
Contributor Author

@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).

@ngreenwald
Copy link
Member

I agree, let's average them, then name them avg_norm_func_450, etc.

@alex-l-kong alex-l-kong requested a review from ngreenwald June 30, 2023 21:46
Copy link
Member

@ngreenwald ngreenwald left a 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

@alex-l-kong alex-l-kong requested a review from ngreenwald July 3, 2023 19:34
@ngreenwald ngreenwald merged commit 97e16c8 into main Jul 3, 2023
@ngreenwald ngreenwald deleted the norm_workflow branch July 3, 2023 20:26
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.

Update tuning curve process for normalization Default normalization curves
4 participants