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

Documentation page improvements #150

Merged
merged 36 commits into from
Sep 12, 2022

Conversation

freemansw1
Copy link
Member

@freemansw1 freemansw1 commented Jun 30, 2022

This PR brings over most of the documentation page improvements from #127 , and should pair nicely with #138 .

  • Have you followed our guidelines in CONTRIBUTING.md?
  • Have you self-reviewed your code and corrected any misspellings?
  • Have you written documentation that is easy to understand?
  • Have you written descriptive commit messages?
  • Have you added NumPy docstrings for newly added functions?
  • Have you formatted your code using black?
  • If you have introduced a new functionality, have you added adequate unit tests?
  • Have all tests passed in your local clone?
  • If you have introduced a new functionality, have you added an example notebook?
  • Have you kept your pull request small and limited so that it is easy to review?
  • Have the newest changes from this branch been merged?

To get his feet wet on reviewing, I've requested @pjmarinescu as a reviewer here. As one of the original tobac devs, I think his perspective on this will be really useful.

@freemansw1 freemansw1 added enhancement Addition of new features, or improved functionality of existing features documentation Updates and improvements to documentation and examples labels Jun 30, 2022
@freemansw1 freemansw1 added this to the Version 1.4 milestone Jun 30, 2022
@freemansw1
Copy link
Member Author

Ah- I've now added readthedocs building to PRs, which has just failed! It builds locally, but clearly, there is an issue with readthedocs. I'll work on revising the PR shortly.

@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #150 (4074eae) into RC_v1.4.0 (90fb3ff) will increase coverage by 3.24%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           RC_v1.4.0     #150      +/-   ##
=============================================
+ Coverage      31.69%   34.93%   +3.24%     
=============================================
  Files             11       11              
  Lines           2051     2098      +47     
=============================================
+ Hits             650      733      +83     
+ Misses          1401     1365      -36     
Flag Coverage Δ
unittests 34.93% <ø> (+3.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tobac/analysis.py 7.91% <0.00%> (ø)
tobac/segmentation.py 75.24% <0.00%> (ø)
tobac/centerofgravity.py 6.25% <0.00%> (ø)
tobac/plotting.py 3.08% <0.00%> (+<0.01%) ⬆️
tobac/feature_detection.py 70.71% <0.00%> (+2.64%) ⬆️
tobac/utils.py 46.75% <0.00%> (+5.71%) ⬆️
tobac/tracking.py 64.48% <0.00%> (+8.93%) ⬆️
tobac/testing.py 94.55% <0.00%> (+10.38%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Following the suggestion here: https://stackoverflow.com/a/56448499/629110 , I switched the new `conf.py` file to switch the master_doc to `index`
@freemansw1
Copy link
Member Author

freemansw1 commented Jun 30, 2022

Thanks to readthedocs, you can preview these new documents here: https://tobac--150.org.readthedocs.build/en/150/ (or by hitting details on the readthedocs action).

Also, given the relation to #138 , I've requested @snilsn as a third reviewer, if he is interested and has time.

@freemansw1 freemansw1 mentioned this pull request Jul 3, 2022
@freemansw1
Copy link
Member Author

I've gone through and made some substantial changes to this PR, including a new page describing some of the feature detection parameters and three new Jupyter notebooks addressing the same. I do plan to expand on this and do similar work on both the segmentation section and tracking section.

@freemansw1
Copy link
Member Author

Note that I've updated the documentation with the segmentation discussion here. I'm going to wait to request re-reviews until I add in the tracking discussion.

Copy link
Collaborator

@snilsn snilsn left a comment

Choose a reason for hiding this comment

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

Very good work!

I left a few comments, mostly just small things or thoughts.

doc/threshold_detection_parameters.rst Outdated Show resolved Hide resolved
doc/threshold_detection_parameters.rst Outdated Show resolved Hide resolved
doc/analysis.rst Outdated Show resolved Hide resolved
doc/data_input.rst Outdated Show resolved Hide resolved
doc/feature_detection_output.rst Outdated Show resolved Hide resolved
doc/tracking_base_out_vars.csv Outdated Show resolved Hide resolved
Copy link
Member

@JuliaKukulies JuliaKukulies left a comment

Choose a reason for hiding this comment

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

Great @freemansw1 ! The basic examples are very nice and clear and I do not have much to add. Feel free to consider or ignore my minor suggestions. And yes, last check then with the changes for tracking.

I know it has not been part of this PR, but I am also wondering if we should update the description for Example notebooks with the right zenodo link to the data and a link to the v2.0 tutorials in one go?

doc/installation.rst Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/data_input.rst Outdated Show resolved Hide resolved
doc/data_input.rst Outdated Show resolved Hide resolved
@JuliaKukulies
Copy link
Member

Committed the publication page now to this PR. Let me know if you have any comments or requests for changes here!

@JuliaKukulies
Copy link
Member

And sorry, I reverted my first two commits because I still had the old index.rst in my branch. Now everything should be clean and the last three commits contain the additions for the publication page. Feel also free to move it somewhere else if you think it is not the best location to have it after the examples.

@freemansw1
Copy link
Member Author

I believe that I've addressed all the comments here.

I haven't yet finished (or even really started) the documentation on segmentation or tracking. I do want to do that, but I'm inclined to do that as a separate PR off of this one, once this one is merged? That will keep the review process simple. Let me know what you think @snilsn @JuliaKukulies @pjmarinescu

@JuliaKukulies
Copy link
Member

Looks good @freemansw1, I am happy with the changes! And I agree that you it would be easier to include the documentation on segmentation and tracking in a separate PR as this one is already quite big.

@snilsn
Copy link
Collaborator

snilsn commented Sep 10, 2022

Good work @freemansw1 , I'm happy with these edits and also with splitting the work on the documentation into two PRs.

@freemansw1
Copy link
Member Author

Thanks @snilsn and @JuliaKukulies . Given that @pjmarinescu has comments, I'll wait to hear from him before merging.

@pjmarinescu
Copy link
Collaborator

Agree regarding splitting into to PRs.

Copy link
Collaborator

@pjmarinescu pjmarinescu left a comment

Choose a reason for hiding this comment

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

Thanks for considering all my comments Sean. Nice job.

@freemansw1 freemansw1 merged commit f5ad032 into tobac-project:RC_v1.4.0 Sep 12, 2022
@freemansw1
Copy link
Member Author

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Updates and improvements to documentation and examples enhancement Addition of new features, or improved functionality of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants