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

Generate docs #56

Merged
merged 12 commits into from
Aug 8, 2024
Merged

Generate docs #56

merged 12 commits into from
Aug 8, 2024

Conversation

maestroque
Copy link
Contributor

@maestroque maestroque commented Jul 11, 2024

Closes #

Generate a documentation page for phys2denoise, including showcase of the utilization of the Physio object within the package

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

@maestroque maestroque added the Documentation This issue or PR is about the documentation label Jul 11, 2024
@maestroque maestroque self-assigned this Jul 11, 2024
@maestroque maestroque changed the title [WIP] Generate docs Generate docs Jul 23, 2024
@m-miedema m-miedema self-requested a review July 24, 2024 16:17
Copy link
Contributor

@me-pic me-pic 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 @maestroque for working on that ! It's really nice to have doc for phys2denoise 🎉 Just left some comments in my review. Let me know if you have any question / want to discuss them !


.. _basic_installation:

Developer installation
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if there is a reason to only have the dev installation here


# Load the physiological data
sample_rate = 1000
physio = io.load_physio('path/to/physiological/data', fs=sample_rate)
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point it would be nice to provide actual data for people to play around

@@ -0,0 +1,10 @@

Contents
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for now it is ok, but at some point it would be necessary to provide a bit more context/use cases for that package. You can check peakdet doc for an example


The benefit of using a Physio object other than the encapsulation of all the desired parameters in a single object is the fact that
the object retains a history of all the operations performed on it. This allows for easy debugging and reproducibility of the results.
For further information refer to the :py:mod:`physutils` documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm not aware of it, but it looks like for the moment there is not physutils documentation. So since you are referring to it, I'm wondering if we should work on the physutils docs before merging that PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can generate some docs only including API for the time being.


from phys2denoise.metrics.chest_belt import respiratory_variance_time

# Given that the respiratory signal is stored in `data`, the peaks in `peaks`, the troughs in `troughs`
Copy link
Contributor

Choose a reason for hiding this comment

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

Here do you mean the respiratory signal is stored in physio ? However I think it might be less confusing to keep the name you used in the comment and use data in the function i.e. respiratory_variance_time(data, peaks, troughs, sample_rate)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also do you think it would be a good idea to show in that example how users would load their data ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think loading the data is really dependent on the format the user is using, idk if it would be fitting here

Without using a Physio object
#############################

However, if the use of :py:mod:`physutils` is not preferred, the metrics can be also computed without it. The following
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it would make more sense to say if the use of the physio object rather than if the use of physutils. What do you think ?

from phys2denoise.metrics.utils import export_metric

RVT = respiratory_variance_time(
resp.data, resp.peaks, resp.troughs, resp.fs, lags=(0, 4, 8, 12)
Copy link
Contributor

Choose a reason for hiding this comment

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

I might have missed it but I don't see where resp was defined previously

@maestroque
Copy link
Contributor Author

@me-pic I implemented your suggestions, it should hopefully be gtg

@me-pic me-pic self-requested a review August 8, 2024 19:31
Copy link
Contributor

@me-pic me-pic left a comment

Choose a reason for hiding this comment

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

@maestroque Thank you ! I've pushed some changes directly on your branch. Just changed some sentence formulation and added the warning about the potential installation issue regarding matplotlib versioning

@me-pic me-pic merged commit d336e42 into physiopy:master Aug 8, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation This issue or PR is about the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants