Closed whedon closed 4 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @bradyrx, @scivision it looks like you're currently assigned to review this paper :tada:.
:star: Important :star:
If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿
To fix this do the following two things:
For a list of things I can do to help you, just type:
@whedon commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@whedon generate pdf
Reference check summary:
OK DOIs
- 10.1016/J.RSE.2018.06.006 is OK
- 10.1038/s41597-019-0083-9 is OK
MISSING DOIs
- None
INVALID DOIs
- None
👋 @sjpfenninger — It looks like you will need to reach out to your reviewers by email to request an update, and propel this along...
@labarba Thanks, yes, I'm on it!
RADWave
Thank you to @CourtneySmith32 and colleagues for submitting this software and manuscript. The documentation is extensive and I found the binder examples impressive and quite useful. I appreciate that all of the binder analysis was done with THREDDS to show that data can be pulled from remote servers and need not be stored locally for this package to work. It's clear that RADWave
serves a need in the community and will make analyzing altimetry data a lot smoother. Nice work!
I have comments below on the software, documentation, and summary paper that should be addressed prior to publication with JOSS. Generally, some more care needs to be taken with package management (testing, CI, etc.) and the code base needs to be cleaned up a bit (consistent naming and returns, linting, etc.). However, once these are addressed I think it's a great fit for the journal.
radwave
. PEP guidelines suggest that python packages should be named in all lowercase, and only use underscores to improve readability (https://www.python.org/dev/peps/pep-0008/#package-and-module-names)./Notebooks/
and /src/RADWave/Notebooks/
? I imagine these can be consolidated into one location. Typically for python, everything from src
can be moved to the main folder.return
statements are necessary throughout the class methods.flake8
or a similar linter on the code. There's currently 100 errors that come through from flake8
on the code base. It might also help to have a column-width limitation for user and developer readability. E.g. black
, or just enforce an 80 or 88 line limit.processingAltimeterData
can become processAltimeterData
and readingAltimeterData
to readAltimeterData
. seriesSeasonMonth
can be renamed something more clear like computeSeasonalCharacteristics
(or something less verbose). timeSeries
should be something more descriptive like generate_time_series
or something in camelCase. It currently reads like a class attribute.'H', 'T', 'P', 'E' and 'Cg'
) should be clearly defined in the docs and/or in the docstring for plotTimeSeries
.df
's from functions instead of just adding them as object attributes. E.g. from timeSeries
and close2Track
. Generally it's intuitive for the user to be returned the computation result from a method like these. Since there's no __repr__
on the main class, the user has no interaction with their object and can't see when timeSeries
has already been computed. Further, there's inconsistencies with seriesSeasonMonth
which returns the df
, but the former two mentioned methods which don't.cycloneAltiPoint
should probably be renamed to plotCycloneAltiPoint
for consistency and clarity.Could the lead author please outline their contribution and Dr. Vila-Concejo's contribution to the software? The repository suggests that all of the code was written by @tristan-salles, and the Sphinx copyright and contact email on the documentation goes to @tristan-salles as well.
[X] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
I think the installation page is comprehensive and I appreciate the multiple methods to install. However, I would also suggest registering RADWave
with conda-forge
, since this is probably the best package management solution out there right now.
[ ] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
The only testing available is testInstall.py
, which seems to just test that RADWave
can be imported and the main THREDDS method run. First, it's currently breaking for me (see https://github.com/pyReef-model/RADWave/issues/1).
Secondly, RADWave
should have unit testing well beyond this. I would suggest having, e.g. a pytest
suite that is run with continuous integration, such as Travis, on all PRs. You can use coveralls
or a similar service to check how much of the code base is covered by testing. At the minimum, I would test all of the methods in the waveAnalysis
object and ensure that they run without breaking. You should probably use pytest.parametrize
to loop through different keyword options as well.
If the authors don't have too much experience with this level of unit testing, I suggest checking out our testing suite at climpred
: https://github.com/bradyrx/climpred/tree/master/climpred/tests. I think we have generally minimal examples there, which are easier to base testing off of than a bigger package like numpy
or xarray
.
[ ] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
Please see comments below. I suggest that the paper be carefully revised in a few locations for grammar and clarity.
[ ] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
Please see comments below. I suggest the addition of at least one reference in the summary.
Consequently, analysis of this long-term temporal record can lead to significant insights into inter-annual, seasonal and decadal variations in wave yearly seasonality and decadal trend.
I'm not exactly sure what is being said here. E.g., what is "wave yearly seasonality"? How do decadal trends fit into this picture? Perhaps that analyzing the record can lead to insights in interannual, seasonal, and decadal variations in wave statistics (or something similar).
RADWave allows to query over a range of spatial and temporal scales altimeter parameters in specific geographical regions and subsequently calculates significant wave heights, periods, group velocities, average wave energy densities and wave energy fluxes.
pandas
, numpy
, matplotlib
, etc. And that RADWave
combines this all into a simple object-oriented framework. Hi Riley,
Thanks very much for your comments and feedback, they all make sense. Will get on it soon!
Thanks, Courtney
On Sat, Mar 7, 2020 at 10:06 AM Riley Brady notifications@github.com wrote:
Review of RADWave
Thank you to @CourtneySmith32 https://github.com/CourtneySmith32 and colleagues for submitting this software and manuscript. The documentation is extensive and I found the binder examples impressive and quite useful. I appreciate that all of the binder analysis was done with THREDDS to show that data can be pulled from remote servers and need not be stored locally for this package to work. It's clear that RADWave serves a need in the community and will make analyzing altimetry data a lot smoother. Nice work!
I have comments below on the software, documentation, and summary paper that should be addressed prior to publication with JOSS. Generally, some more care needs to be taken with package management (testing, CI, etc.) and the code base needs to be cleaned up a bit (consistent naming and returns, linting, etc.). However, once these are addressed I think it's a great fit for the journal. General Comments/Questions
- I would consider renaming the package to radwave. PEP guidelines suggest that python packages should be named in all lowercase, and only use underscores to improve readability ( https://www.python.org/dev/peps/pep-0008/#package-and-module-names).
- Does this only work with the setup from AODN, or will it work with any THREDDS altimetry data? From the summary paper, it seems like AODN hosts all altimetry mission output, so this might not be relevant. If that's the case, I would suggest making this clear in the docs. Currently the docs/manuscript say AODN is used to illustrate the package capabilities, but has any testing been done on other servers?
- Why are the notebooks/tutorial replicated in both /Notebooks/ and /src/RADWave/Notebooks/? I imagine these can be consolidated into one location. Typically for python, everything from src can be moved to the main folder.
- I would encourage the use of a CI service like Travis for this package. This ensures that the package can be built/installed for a given PR's updates, and will run a testing suite as well (which I suggest in the next section).
- Is 30 days the common moving average window in the field for wave time series? Currently, the 30-day average is hard-coded into the class methods for plotting time series. It might be useful for the user to be able to designate the moving average window length.
Code Comments
- I don't think the empty return statements are necessary throughout the class methods.
- I would strongly recommend running flake8 or a similar linter on the code. There's currently 100 errors that come through from flake8 on the code base. It might also help to have a column-width limitation for user and developer readability. E.g. black, or just enforce an 80 or 88 line limit.
- I think functions can generally be renamed for clarity. E.g. processingAltimeterData can become processAltimeterData and readingAltimeterData to readAltimeterData. seriesSeasonMonth can be renamed something more clear like computeSeasonalCharacteristics (or something less verbose). timeSeries should be something more descriptive like generate_time_series or something in camelCase. It currently reads like a class attribute.
- Time series options ('H', 'T', 'P', 'E' and 'Cg' ) should be clearly defined in the docs and/or in the docstring for plotTimeSeries.
- I would consider returning the derived df's from functions instead of just adding them as object attributes. E.g. from timeSeries and close2Track. Generally it's intuitive for the user to be returned the computation result from a method like these. Since there's no repr on the main class, the user has no interaction with their object and can't see when timeSeries has already been computed. Further, there's inconsistencies with seriesSeasonMonth which returns the df, but the former two mentioned methods which don't.
- cycloneAltiPoint should probably be renamed to plotCycloneAltiPoint for consistency and clarity.
Review Checklist General checks
- Contribution and authorship: Has the submitting author ( @CourtneySmith32 https://github.com/CourtneySmith32) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
Could the lead author please outline their contribution and Dr. Vila-Concejo's contribution to the software? The repository suggests that all of the code was written by @tristan-salles https://github.com/tristan-salles, and the Sphinx copyright and contact email on the documentation goes to @tristan-salles https://github.com/tristan-salles as well. Documentation
-
Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
I think the installation page is comprehensive and I appreciate the multiple methods to install. However, I would also suggest registering RADWave with conda-forge, since this is probably the best package management solution out there right now.
Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
The only testing available is testInstall.py, which seems to just test that RADWave can be imported and the main THREDDS method run. First, it's currently breaking for me (see pyReef-model/RADWave#1 https://github.com/pyReef-model/RADWave/issues/1).
Secondly, RADWave should have unit testing well beyond this. I would suggest having, e.g. a pytest suite that is run with continuous integration, such as Travis, on all PRs. You can use coveralls or a similar service to check how much of the code base is covered by testing. At the minimum, I would test all of the methods in the waveAnalysis object and ensure that they run without breaking. You should probably use pytest.parametrize to loop through different keyword options as well.
If the authors don't have too much experience with this level of unit testing, I suggest checking out our testing suite at climpred: https://github.com/bradyrx/climpred/tree/master/climpred/tests. I think we have generally minimal examples there, which are easier to base testing off of than a bigger package like numpy or xarray.
Software paper
-
Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
Please see comments below. I suggest that the paper be carefully revised in a few locations for grammar and clarity.
References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html#citation_syntax ?
Please see comments below. I suggest the addition of at least one reference in the summary.
Summary
- In the first sentence, I would restructure it to either say that satellite radar altimeters are excellent instruments or that satellite radar altimetry is an excellent remote sensing technique.
- In the second sentence, gramatically it should be restructured to say "These measurements can be used in conjunction with one another ..."
- As in the docs, I would suggest citing Chelton 2001 or something similar to back up the gap in satellite operation.
- Please rewrite the following sentence for clarity:
Consequently, analysis of this long-term temporal record can lead to significant insights into inter-annual, seasonal and decadal variations in wave yearly seasonality and decadal trend.
I'm not exactly sure what is being said here. E.g., what is "wave yearly seasonality"? How do decadal trends fit into this picture? Perhaps that analyzing the record can lead to insights in interannual, seasonal, and decadal variations in wave statistics (or something similar). Main Text
- Please reformulate this sentence as well. It seems like there are words missing (e.g. "allows the user to query"?):
RADWave allows to query over a range of spatial and temporal scales altimeter parameters in specific geographical regions and subsequently calculates significant wave heights, periods, group velocities, average wave energy densities and wave energy fluxes.
- Perhaps it would be good to follow the line about there being no open-source code available for this purpose with a suggestion that users currently have to download the data manually (or write by hand THREDDS retrieval code) and write custom analysis and visualization functions using pandas, numpy, matplotlib, etc. And that RADWave combines this all into a simple object-oriented framework.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2083?email_source=notifications&email_token=AL3QBXLTP3OYQHECOCF2H6LRGF6YJA5CNFSM4KRMARIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEODELUI#issuecomment-596002257, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL3QBXK3NGEJIWQKKTDFC6LRGF6YJANCNFSM4KRMARIA .
Review checklist for @scivision
Review comments
It looks like a lot of improvements were made recently, including in response to the first reviewer's comments. The biggest gaps to be "accepted" have been already addressed as a result.
👋 @scivision - you already have a checklist in the first comment in this review thread - you shouldn't create your own
Thanks Daniel, for some reason I don't have access to that, I can't edit that comment. It may be an invitation issue. I can move that information to the first comment upon gaining access
@whedon re-invite @scivision as reviewer
Sorry, I couldn't re-invite @scivision.
@danielskatz hm, any idea what's going wrong here?
I had my status set to Away, that might have blocked the invite. I just turned off Away.
@scivision - can you check again now? @openjournals/dev - if not, can you help?
I think the invite has to be sent again, now that my status in Github is not away
We would like to thank @bradyrx for his really useful and helpful comments. They have all helped to improve significantly the quality of the package!
Below is a point by point revisions of his suggestions.
General Comments/Questions
we have changed the package name to radwave as suggested by the reviewer.
We have removed the duplicated tutorials.
We are now using Travis with pytest for RADwave
Monthly (30 days) average window is commonly used for wave time series analysis saying that one might want to use another time interval and following reviewer’s comment we have modified the code to allow user to pick a specific number of days in the function timeSeries that now accepts an argument: def timeSeries(self, days=30)
As pointed out by the reviewer, the package in its current form only works with AODN altimetry data. In a future version of the code we could make it work with other THREDDS server but as AODN currently hosts all altimetry missions this is not an issue for now. Following reviewer advice we have making it clear in the docs.
Code Comments
As suggested by the reviewer, we have removed the empty return statements.
Following reviewer’s comment we have used flake8 and black to reformat the main code contained in altiwave.py. There are still some line running over the 80 line mark but they are mostly comments and documentation related parts of the files.
We have renamed all the functions based on the reviewer’s comment: e.g. processAltimeterData, readAltimeterData, computeSeasonalCharacteristics and generate_time_series.
We have added the options in the docs and the docstring for clarity.
Following reviewer’s comment we are now returning derived data frames for the generate_time_series and close2Track functions.
The function has been renamed accordingly.
Review Checklist
General checks
The submitting author CS designed an initial version of RADWave as part of her Honours under the supervision of AVC and TS. Subsequently TS wrapped the main functions and extended the code functionalities. The tests were designed by all the three co-authors.
Documentation
The package is available via pypi and can easily be installed in Anaconda using the pip command. We have decided to leave it as is for now as this does not seem to add an extra installation steps to the user as far as we can see plus it limits the number of packages that we will have to maintain in the future.
This has been fixed, we now use Travis and pytest for automated tests.
+ Secondly, RADWave should have unit testing well beyond this. I would suggest having, e.g. a pytest suite that is run with continuous integration, such as Travis, on all PRs. You can use coveralls or a similar service to check how much of the code base is covered by testing. At the minimum, I would test all of the methods in the waveAnalysis object and ensure that they run without breaking. You should probably use pytest.parametrize to loop through different keyword options as well. If the authors don't have too much experience with this level of unit testing, I suggest checking out our testing suite at climpred: https://github.com/bradyrx/climpred/tree/master/climpred/tests. I think we have generally minimal examples there, which are easier to base testing off of than a bigger package like numpy or xarray.
Following reviewer’s comment we have added the pytest suite to our package and use Travis for continuous integration as well as coveralls.
Software paper
We have followed reviewer’s comment and changed the first sentence accordingly.
We have modified the sentence and added the Chelton 2001 reference.
The sentence has been modified accordingly.
We have followed reviewer’s comment and changed these sentences accordingly in the paper and the documentation.
We have added a sentence about this in the paper.
Thank you for the quick turnaround @tristan-salles! I suggest that this is accepted/published after addressing the two minor comments below:
We have renamed all the functions based on the reviewer’s comment: e.g. processAltimeterData, readAltimeterData, computeSeasonalCharacteristics and generate_time_series.
I would make it generateTimeSeries
for consistency with camelCase. I should have noted that.
While domain experts are able to extract satellite radar altimetry dataset, they currently have to download the data manually and write custom analysis and visualization functions using pandas, numpy, matplotlib to extract wave characteristics.
The above is from the updated text. There's still some grammatical issues here. I would change it to:
"While domain experts are able to assess satellite radar altimetry datasets, they currently have to download the data manually and write custom analysis and visualization functions using, e.g., numpy
, pandas
, and matplotlib
to extract wave characteristics.
And here's some ranting thoughts on package management below. You don't need to address these comments, but I thought they might help moving forward.
Following reviewer’s comment we have used flake8 and black to reformat the main code contained in altiwave.py. There are still some line running over the 80 line mark but they are mostly comments and documentation related parts of the files.
We use a black
line limit of 88, and impose that on black
and flake8
in our setup.cfg file here: https://github.com/bradyrx/climpred/blob/master/setup.cfg. 80 ends up pretty short. Anyways, with whatever line limit you use, you can manually force your comments to adhere to this limit. For long comment lines just make them multi-lined. For docstring things (like arguments) if they are multiline, just tab out the second line like a hanging indent.
This all sounds really tedious but makes the code more readable for future developers. We also have a run_linter.sh
script (https://github.com/bradyrx/climpred/blob/master/ci/run-linter.sh) that we run at the end of our Travis to make sure everything looks good for new code. Note that we add all of our code through Pull Requests so that one can't merge the new code until it is cleaned up like this.
Not that climpred
is the bible for this. :) We've set up all of these CI/pre-commit workflows from advice from other packages. It's tedious to get going, but then makes the workflow very straight forward. pre-commit
totally changed our development (https://github.com/bradyrx/climpred/blob/master/.pre-commit-config.yaml). It's worth trying out to see how awesome it is! Once you do pip install pre-commit
, have a config file like I linked in your repo and do pre-commit install
. Then you can set up a bunch of rules (like in my example) and every time you try to commit code it will run those checks. The black
/flake8
hook on there just runs it automatically as you commit the code so you don't even have to think about it.
Thanks! The 2 comments have been resolved and the docker image regenerated and tested as well as the binder one! Again thanks for all the feedbacks and insights!
Thanks @scivision for the reviews & suggestions. We have addressed all your comments.
We have moved the setup.py file to the topmost level of the repo.
We have changed the classifier to be set from Python 3.5 to 3.8
We have changed round to pytest.approx as suggested by the reviewer and have checked that Travis CI is still building without issues. on your comments we
Hi @sjpfenninger, I think we have answered to all the reviewers comments.
Dear authors and reviewers
We wanted to notify you that in light of the current COVID-19 pandemic, JOSS has decided to suspend submission of new manuscripts and to handle existing manuscripts (such as this one) on a "best efforts basis". We understand that you may need to attend to more pressing issues than completing a review or updating a repository in response to a review. If this is the case, a quick note indicating that you need to put a "pause" on your involvement with a review would be appreciated but is not required.
Thanks in advance for your understanding.
Arfon Smith, Editor in Chief, on behalf of the JOSS editorial team.
Hi @sjpfenninger, is there anything else that needs to be done on our side?
@tristan-salles Apologies for the delay, I will likely get to this next week.
Thanks @sjpfenninger for letting us know!
@bradyrx @scivision Thanks both for your reviews, can you confirm whether you are happy with the changes made in response to your comments?
@whedon generate pdf
@scivision You did not tick "Installation: Does installation proceed as outlined in the documentation?" in your checklist -- does that mean there are issues with installing the package?
@tristan-salles You seem to have duplicated setup.py and setup.cfg (it's at both the top level and inside src/) -- are you sure this is intended? It ought to only be in one place.
@sjpfenninger , I am happy with the revisions. Thanks!
@sjpfenninger, you were right, I did forget to remove the setup files in the src. I have made the changes to ensure that there were no more duplications.
@sjpfenninger thanks all set, I checked install box
@tristan-salles I suggested some minor changes to the paper here: https://github.com/pyReef-model/RADWave/pull/2
@tristan-salles Your only tagged release on GitHub is v0.0.2 but PyPI has v0.1.1. Can you make sure you have a tagged release for the most recent version, and report back the correct version number here? Please then make an archive of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive. For the Zenodo/figshare archive, please make sure that:
Once that's done, we can move forward with accepting this!
@whedon set v1.0.0 as version
OK. v1.0.0 is the version.
@whedon set 10.5281/zenodo.3733848 as archive
OK. 10.5281/zenodo.3733848 is the archive.
@whedon accept
Attempting dry run of processing paper acceptance...
Reference check summary:
OK DOIs
- 10.1016/J.RSE.2018.06.006 is OK
- 10.1038/s41597-019-0083-9 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.
Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/1398
If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/1398, then you can now move forward with accepting the submission by compiling again with the flag deposit=true
e.g.
@whedon accept deposit=true
@CourtneySmith32 @tristan-salles Thanks both, this is ready to go as far as I'm concerned. And thanks @bradyrx and @scivision for your work reviewing!
Thanks all for your reviews! This is awesome!
@whedon accept deposit=true
Doing it live! Attempting automated processing of paper acceptance...
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨
Here's what you must now do:
Party like you just published a paper! 🎉🌈🦄💃👻🤘
Any issues? notify your editorial technical team...
Submitting author: @CourtneySmith32 (Courtney Smith) Repository: https://github.com/pyReef-model/RADWave Version: v1.0.0 Editor: @sjpfenninger Reviewer: @bradyrx, @scivision Archive: 10.5281/zenodo.3733848
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@bradyrx & @scivision, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @sjpfenninger know.
✨ Please try and complete your review in the next two weeks ✨
Review checklist for @bradyrx
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @scivision
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper