aminsinichi / wearable-hrv Goto Github PK

• The docstring formatting is inconsistent. Some functions use a numpydoc format while others use a GoogleDoc (?) style format (e.g.

  wearable-hrv/wearablehrv/individual.py

  Line 1552 in ac22c37

  """

  )
• It would be great, if a rendered API docu (e.g. using RTD) would exist to easier browse the available functions
• The main Documentation example should be restructured, that it can be run without modification end-to-end, so that new users can play around with the functionality right away. For this I would suggest replacing, the "undefined" paths with paths to the example file, splitting the docs up into multiple files (1 showing the main functionality and multiple others showing specific functionality, e.g. pre-processing for a specific system)
• Ideally, rendered versions of the example notebook should be hosted somewhere (e.g. as part of RTD page), so that potential users can see the functionality "in action" without installing the package
• If you want to be "fancy" with the handling of your example files, consider using https://pypi.org/project/pooch/ to handle downloading your example files.
• The current documentation notebook has a syntax error in the first cell (missing ! to mark the first line as command)
• Ideally add a test that executes your documentation example to make sure that it is working as expected.

The following functions generate warning messages due to dependency package updates:

• individual.import_data ()
• individual.chop_data ()
• individual.display_changes()
• group.matrix_plot()
• group.density_plot()
• group.heatmap_plot() It does not produce a warning, but seems like something has changed.
• group.bonferroni_correction_icc()

Using pip install to install the package in a clean env results in errors during import.

Specficially, ImportError: cannot import name 'LombScargle' from 'astropy.stats'. This is likely related to the deprecation mentioned here (lightkurve/lightkurve#535)

It seems like the package does not support recent astropy versions. That should be specified in the setup.py to ensure that compatible versions are installed (or even better, the newer versions of astropy should be supported).

Downgrading astropy to <6.0 fixed the issue for now

At the moment the testfiles try to add hardcoed paths to the sys.path

That should be adapted to make it possible to run the tests on every system without modifieing the files

While reading through the paper, I was missing a little bit more thoprough comparison to other available packages to clearer highlight what this package adds. E.g. how much easier is using this package than using the hrv-analysis package directly.

Also It would be great, if you could add citations to the paper that provide support for the evaluation metrics that the package provides. Basically, to provide some justification why you chose the set of metrics and plots that are included in the package for comparison of the systems.

In the paper and the package the term CRITERION DEVICE is used. At least for me (and I also couldn't find anything with a quick google search), the use of the term in this context was new. I think a more common name would be "reference device" or "ground-truth device"

Great job publisheing the package on Pypi!

Here a couple of suggestions to further improve packaging and distribution:

• Your package should have an internal version identifier (usually a version const in the init) that corresponds to the package version, so that users can check which version they have installed
• You should associate each Pypi release with a git tag (and push the tag to Github) to make sure that people can browse the code of exactly the version they are currently running (e.g. when reporting bugs)
• Optionally, you can create a Github release for every version
• Optionally, you can streamline the process, by using Github actions to automatically publish on release or on tag
• You should consider adding a changelog. Otherwise, new versions and their impact are meaningless to users.

Overall the code quality looks great, but here are a couple of smaller things that could be improved:

• Use of Path instead of strings for file/folder paths -> This will solve a bunch of issues caused by the differences between operating systems. For example it would make .replace('\\', '/') obsolote in the example
• At the moment the files are huge. It might help overall readibility to split the large files into logical chunks
• "random" and repeated comments (For example at the beginning of the individual file)
• Inconsistent and unusuale formatting choices (in particular the space before the opening bracket of a function) can be confusing. I would highly recommend to stick to official code guidelines and enforce them by using ruff or black

As per JOSS requirements a package should have Contribution Guidelines:

Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support