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

Docstrings for plotting.py #155

Merged
merged 5 commits into from
Jul 11, 2022
Merged

Conversation

snilsn
Copy link
Collaborator

@snilsn snilsn commented Jul 6, 2022

This PR contains revised docstrings for the plotting.py functions I've gotten to work so far.

Unfortunately, this time there are more functions which are not covered yet. To keep track of that I use this table

I created a notebook of my experiments with the functions that are marked as 'broken?'. I suspect some of them to have bugs, maybe resulting from changes in other parts of tobac. Others I perhaps use in the wrong way. None of these I saw in use in any of the examples.

This is probably worth a separate issue, like suggested in #146. We should discuss whether they should still be included in this PR.

  • 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 formatted your code using black?
  • Have all tests passed in your local clone?
  • 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?

Not needed:

  • Have you added NumPy docstrings for newly added functions?
  • If you have introduced a new functionality, have you added adequate unit tests?
  • If you have introduced a new functionality, have you added an example notebook?

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #155 (6807a98) into RC_v1.4.0 (3ee91cc) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           RC_v1.4.0     #155      +/-   ##
=============================================
- Coverage      32.23%   32.19%   -0.05%     
=============================================
  Files             11       11              
  Lines           2072     2050      -22     
=============================================
- Hits             668      660       -8     
+ Misses          1404     1390      -14     
Flag Coverage Δ
unittests 32.19% <ø> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
tobac/plotting.py 3.08% <ø> (+<0.01%) ⬆️
tobac/segmentation.py 75.24% <0.00%> (+6.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ee91cc...6807a98. Read the comment docs.

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

Thanks Nils! I've requested @JuliaKukulies and myself for review. I'll try to get this reviewed before the developers' meeting. Agreed that we should discuss the plotting.py functions; I had no idea that this many of them were broken. I'm happy for you to open the issue, or I can do that!

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.

Thank you @snilsn for these comprehensive documentation improvements! This was for sure not an easy task, and based on your notebooks it seems like the plotting functions need some general revisions (good idea to open an issue for that!), which probably will require some update of these docstrings at a later point. I would, however, be happy to get these changes in for v1.4 as these are a nice complement to your PR #138. Again, just a few comments and suggestions, mainly typos and some clarifications

tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
tobac/plotting.py Outdated Show resolved Hide resolved
@snilsn
Copy link
Collaborator Author

snilsn commented Jul 8, 2022

Thanks for your feedback, @JuliaKukulies, most of your suggestions should be covered by the last commit.

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.

All right, approved from my side! Thanks for the changes @snilsn

I guess we can see over that codecov fails here - maybe discuss later how to set the threshold for accepted decrease in coverage.

tobac/plotting.py Outdated Show resolved Hide resolved
@freemansw1 freemansw1 changed the base branch from dev to RC_v1.4.0 July 8, 2022 14:52
Copy link
Member

@freemansw1 freemansw1 left a comment

Choose a reason for hiding this comment

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

Just a minor comment. Great work again.

tobac/plotting.py Show resolved Hide resolved
@freemansw1
Copy link
Member

OK- I've opened a new issue for the broken functions (#158).

@freemansw1
Copy link
Member

Now that we have two approvals and this is passing non-coverage tests (see #159), I'm happy to merge this if you are happy for it to be merged @snilsn .

@snilsn
Copy link
Collaborator Author

snilsn commented Jul 11, 2022

Yes all good, please go ahead @freemansw1

@freemansw1 freemansw1 merged commit 6e73fca into tobac-project:RC_v1.4.0 Jul 11, 2022
@snilsn snilsn deleted the plot_docs branch July 11, 2022 15:41
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.

3 participants