Closed paudetseis closed 1 month ago
Hi @paudetseis
Thank you for your review of our repository. Your comments are very helpful in improving the accessibility of the software.
Thank you, @akinremisa, for making those revisions to the software. I can now install rfsed
without a glitch using both pip
and cloning. The documentation is also much improved, and the html doc is highly appreciated. I still have a few suggestions for improving the user's experience:
pytest
after installation will produce very different results whether the software is installed via pip
or through cloning, because the source code is not in the current directory when installed via pip
. I suggest you either tell the user where to go to run the tests depending on the install method, or you specify that tests are only available for those who clone the code. tqdm
is unavailable (not installed). This should be fixed in the installation (setup.cfg). Note, I am leaving a couple of checkboxes open in my review, that I will check once these comments have been addressed.
Perfect, thanks @paudetseis
Hi @paudetseis
Thank you for the new comments. I now addressed them in the repository and also provide the details of the change made below.
The results of the automated tests (waveform fitting) look as such because they use smaller discretization to make them run faster. On running the tests, this is now more clearly described in the documentation. The pytest can be used in the package directory when rfsed is installed by cloning the repository. There is an option to install the automated files locally and then run the tests. This is described in the documentation.
'tqdm' is a dependency of rf and should be installed automatically along side other dependencies. However, it is also now included in the setup.cfg file.
Because I use Jupyter notebook viewer that is directly linked to the example files in the repository, the same version (executed) is kept for both the online viewing and the repository itself.
I now use a single station data for all the examples (Station OPLO). The station is overlying sedimentary rock and the estimation of the Moho depth and Vp/Vs is poorly constrained when using one-layer H-k stacking, Sequential H-k stacking and the resonance filtering methods. This effect of sediment on the estimation in the examples is not clearly described in the notebook. Also, the waveform fitting approach estimate a more reliable Moho depth and Vp/Vs estimates.
Now, same data (single station - OPLO) is used for all the examples and tests.
Tahnk you @akinremisa I appreciate these changes and can now recommend publication at JOSS. I will now close this comment and mark it as complete. Good work!
Hi @akinremisa,
Thank you for making your code public. It looks like an interesting and useful application, and I would likely end up using it myself or recommending it to collaborators. However, I found the package to be deficient in several regards, and I strongly encourage you to address the following comments and suggestions:
rfsed
usingpip
works fine, but it isn't clear what to do once the package is installed this way. One can then import it usingimport rfsed
, but then what? Because the software is not clearly developed as an API (although it could be, see below) but rather a collection of example notebooks that import therfsed
modules and run the functions, installing withpip
makes little sense to me because those scripts are not available by importingrfsed
, and the user does not have a clear idea where theexamples
folder is located (on my computer they are in/opt/anaconda3/envs/rfsedtest/lib/python3.10/site-packages/rfsed/examples
). If you wish to makerfsed
available via PyPI, you should have a function that installs the notebooks locally (e.g.,import rfsed; rfsed.install_notebooks()
), and there should be documentation to explain this and how to run the tests, etc. Alternatively, you could removerfsed
from PyPI and only provide installation instructions through cloning.conda
environment did not work for me. It appears like theenvironment.yml
file is too restrictive with specific package versions, and this install is not flexible enough. Most users will give up trying to fix the package versions, or you will end up spending lots of time trying to debug other people's environments. I recommend specifying which external packages are minimally required and installing them when creating the conda environment, e.g.,conda create -n rfsed python=3.9 obspyh5=0.6 ...
.examples
folder. In particular, this doesn't work when installing using PyPI. Please either make a full documentation or explain how to install and run the notebooks. However, even the notebooks should be edited to improve clarity (the code is difficult to read and contains a single comment per code cell). Writing proper documentation is essential; it improves the user experience and prevents you from receiving countless requests for help.src/rfsed
folder contains the modules developed (each module being a collection of functions), but these are not listed anywhere, and the user doesn't know what they do. They appear to contain minimal documentation that can be consulted when typing, e.g.,from rfsed import ExtractEq; help(ExtractEq); help(ExtractEq.ExtractEq)
. Because the API information is available, it should be provided on the Documentation page without having to typehelp()
on the command line. I recommend expanding the docstring to ensure the user knows exactly what the function does and how to call it properly. You could even add basic usage or specific examples directly in the docstring.docs
folder and building it using tools like Sphinx (https://www.sphinx-doc.org/en/master/) or Read the Docs (https://readthedocs.com). Note: I see thatsphinx
is required in theenvironment.yml
file, but it's unclear how it's been used here.pep8
style guide (https://peps.python.org/pep-0008/). You could consider usingBlack
for strict Python code formatting (https://github.com/psf/black)Telewavesim
, which is also a collection of modules that contain functions with example notebooks. This is by no means the only or best example, but it will give you some guidance for a package published in JOSS.This issue relates to https://github.com/openjournals/joss-reviews/issues/6612