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

Mph plots #101

Merged
merged 111 commits into from
Jul 12, 2022
Merged

Mph plots #101

merged 111 commits into from
Jul 12, 2022

Conversation

camisowers
Copy link
Contributor

If you haven't already, please read through our contributing guidelines before opening your PR

What is the purpose of this PR?

Closes #87.
Create an example notebook for MPH plots that updates the x-axis to use cumulative total counts for each FOV.
Also is watcher compatible.

Remaining issues

Any formatting fixes or plot design.
The combined csv for all the data has pulse_heights and cum_total_count and stored. If we also want the individual total_counts and/or the FOV number included I can add those as well.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@camisowers camisowers requested a review from ngreenwald May 5, 2022 17:57
templates/example_MPH_plots.ipynb Outdated Show resolved Hide resolved
templates/example_MPH_plots.ipynb Outdated Show resolved Hide resolved
templates/example_MPH_plots.ipynb Outdated Show resolved Hide resolved
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.

It looks like the functionality is all here. However, the goal of the notebooks is to have a clean interface for the user to work with. This means abstracting all of the main code into helper functions that are called. This also allows the code to be re-used by the watcher functions. However, this notebook has everything in individual cells

@camisowers camisowers self-assigned this May 6, 2022
@camisowers camisowers requested a review from ngreenwald May 17, 2022 23:38
@camisowers
Copy link
Contributor Author

I need to add some kind of error or warning for if the CSVs already exist. We could just pull the data from those or compute the mphs again to be safe and overwrite with new files.
I'll also get started on tests.

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.

Nice job pulling out the logic into functions. Some comments about how to make these functions compatible with the watcher

toffy/mph_comp.py Outdated Show resolved Hide resolved
toffy/mph_comp.py Outdated Show resolved Hide resolved
toffy/mph_comp.py Outdated Show resolved Hide resolved
toffy/mph_comp.py Outdated Show resolved Hide resolved
toffy/mph_comp.py Outdated Show resolved Hide resolved
toffy/mph_comp.py Outdated Show resolved Hide resolved
@camisowers camisowers requested a review from ngreenwald June 28, 2022 18:26
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.

Almost there!

toffy/mph_comp.py Show resolved Hide resolved
toffy/mph_comp.py Show resolved Hide resolved
toffy/test_utils.py Outdated Show resolved Hide resolved
@camisowers
Copy link
Contributor Author

The updated watcher notebook was tested against data from creed and ran smoothly.

@camisowers camisowers requested a review from ngreenwald July 11, 2022 18:18
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 final cleanup

README.md Outdated Show resolved Hide resolved
toffy/mph_comp.py Outdated Show resolved Hide resolved
@camisowers
Copy link
Contributor Author

will update the image extraction notebook name and readme link in PR #130

@camisowers camisowers requested a review from ngreenwald July 12, 2022 21:28
@ngreenwald ngreenwald merged commit 9ba59a3 into main Jul 12, 2022
@ngreenwald ngreenwald deleted the MPH_plots branch July 12, 2022 22:01
@camisowers camisowers added the enhancement New feature or request label Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate MPH per count plots
3 participants