-
Notifications
You must be signed in to change notification settings - Fork 6
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
Examples droplet fusion #674
Conversation
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.
Did a quick first pass and made a few notes on things I noticed.
28d1954
to
3cf1351
Compare
a1ba41c
to
eddcefb
Compare
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.
Looking great! I only have a last few tiny requests.
docs/examples/index.rst
Outdated
@@ -20,4 +20,6 @@ For all of the examples, it is assumed that the following lines precede any othe | |||
twlc_fitting/twlc_fitting | |||
reca_fitting/reca_fitting | |||
cas9_kymotracking/cas9_kymotracking | |||
hairpin_fitting/hairping_unfolding |
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.
This is on main. Rebasing to main
would fix this, rather than adding it on this PR. This will only result in a merge conflict. :)
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.
Don't forget to drop this line. Docs won't build with a dead link.
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.
the hairpin example was already merged into this branch. The doc build failed because I had 2 lines in the index file referring to the hairpin notebook, one of which had a spelling error.
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.
Yes, that is what I meant.
243c156
to
ed0b6ee
Compare
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.
Looking good. A few small suggestions. Don't forget to remove the old link, because the docs won't build if you keep it around (see comments).
docs/examples/index.rst
Outdated
@@ -20,4 +20,6 @@ For all of the examples, it is assumed that the following lines precede any othe | |||
twlc_fitting/twlc_fitting | |||
reca_fitting/reca_fitting | |||
cas9_kymotracking/cas9_kymotracking | |||
hairpin_fitting/hairping_unfolding |
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.
Don't forget to drop this line. Docs won't build with a dead link.
1c9d995
to
27e19eb
Compare
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 small last changes. One thing that's curious is that the code seems to generate a different y-label for a figure than the figure you uploaded.
8c830fd
to
08565d4
Compare
08565d4
to
3e0735e
Compare
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 great!
An example of how to analyze a droplet fusion event
Build can be found here: https://lumicks-pylake.readthedocs.io/en/examples_droplet_fusion/examples/droplet_fusion/droplet_fusion.html