jdiasn/lidarwind

Minor comments [JOSS review]

bgailleton opened this issue · 3 comments

bgailleton commented

Dear authors,

Before submitting my overall comments about the contribution, please find bellow minor comments regarding the review for JOSS.

  1. There are a couple of typos in the example notebooks (docs/notebooks). quicklooks and turbulence_6beam_data have path issues as well: files cannot be found or paths are hardcoded. As a more general note, I would suggest to add a bit more details in the notebooks (This is not my field of expertise and I find them a bit confusing).

  2. The documentation's structure is a bit messy and could be clarified/improved. While it is easy to find clear guidelines to install the package and contribute, usage and general descriptions are harder to find. The content index points 3 times to the intro.

  3. The documentation does not describes, even briefly, the API and only points to example notebooks for usage (which is already nice, but does not provide a lot of details).

  4. The manuscript is well written and clear, I would only suggest to rephrase the very last sentence describing future additions. The JOSS paper (I believe) should remain general and up-to-date even in the case of future additions. Maybe rephrasing/extending this sentence around how it can be used to implement other standards, or to point to a dedicated page on the documentation describing the additions post-publication?

Boris

jdiasn commented

Dear @bgailleton , thanks for your comments. Those are valid points. We will address those and ping you in the respective PRs so you can keep track of it.

jdiasn commented

@bgailleton, I believe I addressed all issues listed here. The notebooks were expanded, and a section of rendered notebooks was added to the documentation (https://lidarwind.readthedocs.io/en/latest/notebooks.html). The usage section was expanded to indicate how to use the different classes for pre-processing the data and retrieving wind. The 3 times reference to the intro was solved. In the introduction of the documentation, the user is notified of the availability of an API and a section of rendered docstrings was added to the documentation as well (https://lidarwind.readthedocs.io/en/latest/modules.html#). The paper was updated and expanded following the suggestion from another review.

If you think all points listed in this issue were addressed, you can close this issue.

bgailleton commented

@jdiasn Thanks a lot for addressing all my points. After briefly reviewing the changes, I can confirm do not have further comments or suggestions.