-
Notifications
You must be signed in to change notification settings - Fork 55
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
First part of the revised docstrings #138
Conversation
I was not able to figure out what the functions cell_statistics_all(), cell_statistics() and cog_cell() of analysis.py do and how to use them. If anyone has used them I would be grateful for any advice. |
Hi Nils! I am probably the only one who can help with those functions in analysis.py. It is basically a set of analysis methods for the calculation of specific statistical properties along the cell tracks making use of the segmentation mask. I can have a look at making sure they still work, adding some more detailed documentation to them and putting together a little example notebooks over the next week or so! |
Thanks Max, appreciate it! |
Thanks Nils! I've requested @JuliaKukulies and I as reviewers for now, and I'll try to review this next week. Sorry for the delayed action, it's been busy for all of us. @mheikenfeld do you want to review as well, or would you rather your thoughts be more informal? |
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.
Many thanks to you @snilsn! Your great effort on the improved documentation is much appreciated! Before I approve, I have a few comments and suggestions, but most of them are only minor things or direct corrections of typos or misspellings
Co-authored-by: JuliaKukulies <44163060+JuliaKukulies@users.noreply.github.com>
Co-authored-by: JuliaKukulies <44163060+JuliaKukulies@users.noreply.github.com>
Co-authored-by: JuliaKukulies <44163060+JuliaKukulies@users.noreply.github.com>
Co-authored-by: JuliaKukulies <44163060+JuliaKukulies@users.noreply.github.com>
Co-authored-by: JuliaKukulies <44163060+JuliaKukulies@users.noreply.github.com>
Co-authored-by: JuliaKukulies <44163060+JuliaKukulies@users.noreply.github.com>
Co-authored-by: JuliaKukulies <44163060+JuliaKukulies@users.noreply.github.com>
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.
Great, thank you for the quick response on that! I am happy with all the updates so far. Just waiting for our remaining discussion points and then @freemansw1 may want to have a look as well. Really great work :)
…ying tracking algorithm
Codecov Report
@@ Coverage Diff @@
## dev #138 +/- ##
======================================
Coverage ? 31.59%
======================================
Files ? 11
Lines ? 2048
Branches ? 0
======================================
Hits ? 647
Misses ? 1401
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
In addition to the changes mentioned in the existing discussions, I made an update to the explanation of the subnetwork_size parameter to reflect the impact of adaptive search. |
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 am happy with this now, many thanks @snilsn! The question is only if we want to wait until the missing docstrings in analysis.py
are solved or not. I would almost suggest to go ahead without them, since @freemansw1 initiated a general discussion on future of these functions ( #146 ) and because this is only meant to be the first chunk of improved docstrings, not the complete revision. What is your opinion @freemansw1 ?
@snilsn: Great work, thanks for the work on making the Docstrings much more concise, even in places where it was not that easy to see what the functions are actually doing. @JuliaKukulies, thanks more the detailed review and additional points. I agree that it might be good to merge this in as it is and then look at analysis.py, plotting.py etc. in more detail as part of the discussion started by @freemansw1 in #146, as a lot of it will probably be moved out of the main tobac codebase so that the really thorough documentation of it might be unnecessary work... |
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.
Overall, really excellent job. Just some minor changes here.
Thanks for your feedback, @freemansw1. This new update should address all the remarks you made, and also the remaining open conversations |
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'm happy with this now!
I am happy to merge this now, if you are @snilsn . Are you (and @JuliaKukulies ) happy for us to merge this as a squash merge, or should we merge as a normal merge commit? |
Actually I realized we should wait to merge this until we are staging for v1.4.0, and I am disinclined now from doing a squash merge, so I will plan on merging with a normal commit. |
I would be happy with both options |
I agree that it is good to wait for staging for v1.4.0 and not sure about the squash commit. I think @snilsn has written quite clear commit messages and kept the commits organized so that it is easy to follow what has been changed. It is just quite many commits, because my direct suggestions for typo fixes all come as single commits. Maybe I could find a way to only squash those? That would reduce the number of commits by more than half |
As discussed in our meeting yesterday, I've moved this to target v1.4.0, so I'm now happy for us to merge this as long as you are happy @snilsn . |
Yes, all good. Please go ahead |
This PR contains the revision of the docstrings in analysis, center_of_gravity, feature detection, segmentation, testing and tracking.