Closed AJQuinn closed 1 year ago
Perhaps thats too much for a single comment..! Please let me know if that all makes sense, happy to chat more if anything is worth clarifying or discussing.
Hi @AJQuinn , I believe I have addressed everything in this issue, so I will close it. Please let me know if I haven't addressed anything sufficiently. I am working on updating the documentation for the other issue currently.
Hello - overall I'm impressed with the code quality and writing of this package. Its clear that care has been taken and it appears that the functions and objects handle complicated analyses relatively clearly. The tests are thorough and cover a range of sensible checks. Well done!
My comments are limited to some aspects of code organisation and documentation.
- Code formatting/linting
The code doesn't appear to use a formal style checker or linter, running
flake8 spectral_connectivity/*py
from the route dir gives quite a lot of suggestions - some more reasonable than others.... You may or may not want to adopt formal code-checking as part of the test suite. I find flake8 useful (ignoring some error codes) but this is up to you.Flake does highlight the following points that I think are work checking out.
flake8 spectral_connectivity/*py | grep Missing
These are mainly on properties and methods of objects but are worth completing, a single line of context would do in most cases.
Multitaper.tapers
seems to be partially documented.- Simulations and plotting functions
The simulation functions and plotting utilities in the notebooks are very useful and, I think, deserve to be included in the package as their own submodules. This could really simplify your documentation and be a big help going forward - this one is up to you though, you may not want to maintain this longer term.
I've done something similar in a package based on autoregressive modelling and find it really helpful to be able to quickly generate a known system from within the package.
Maintaining high-quality plotting functions can be a pain, but I think some simple options to help get people started would be great.
- Requirements and versions
The versions of required packages in
setup.py
andenvironment.yml
seemt to be mismatched. Makes sense that setup has the basics and the conda env would have a more complete list.numpy
,pandas
andpytest
have version limits in setup but not in the conda env. I guess conda will resolve this during install but it would certainly be simpler if they matched.- Makefile
Your Makefile seems to be broken. Calling
Make
gives aMakefile:3: *** missing separator. Stop.
error.It seems like the makefile might not be used much from the limited options in it, but I'd recommend either fixing it up or removing it.
xref: https://github.com/openjournals/joss-reviews/issues/4840