Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up[REVIEW]: MetSim: A Python package for estimation anddisaggregation of meteorological data #2042
Comments
This comment has been minimized.
This comment has been minimized.
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @Chilipp, @dsryberg it looks like you're currently assigned to review this paper 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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks @sjpfenninger! I am busy this week with other appointments, but I will do the review on monday in a week. |
This comment has been minimized.
This comment has been minimized.
Hey everyone! My apologies for the delay! I am done with the review and I highly appreciate your work here! MetSim seems to be a very valuable tool for downscaling meterology and I hope that you will provide a methodologic paper (i.e. where you describe the scientific details of your method) at some point as well! The code-base and the documentation of this package is in a good condition and I could install and run the software on my local laptop (Linux). I did not try it on OS X and Ubuntu, and the authors do not state anything about OS-specific requirements (which is something that could be added to the installation instructions). I came across UW-Hydro/MetSim#200 and UW-Hydro/MetSim#201 during the review, and besides that, I have the following comments with reference to the checklist above:
In the paper you state:
In what sense does MetSim improve the performance compared to the previous tools?
The documentation of the
There are tests but it is not described in the documentation, how to run them. This should be mentioned somewhere (e.g. the README file, in the installation instructions or in the Contribution guidelines). I also strongly recommend to measure the coverage of the tests (e.g. using pytest-cov and some free service, such as codecov.io).
Is this enough for a review here? This paragraph does not describe the state of the field, but at least it highlights what is special about MetSim compared to other tools (modern workflow, improved performance, new IO routines, etc., although the statement about improving perfomance has to be revised, see Performance above).
|
This comment has been minimized.
This comment has been minimized.
Hi @Chilipp - thank you for your review! I have addressed your comments above on the master branch. Thanks for catching that timestamp bug! Regarding tests: we do show how to run the tests when installing from source. I'm not 100% sure if that's enough, but because our tests rely on sample datasets, it seems like the easiest way to recommend running them. Let me know if you have other ideas. Regarding performance/state of the field: I added a bit to the performance claim, but the main takeaway is that we have implemented scalable parallelism, which is why we included a section on it. For the section on the state of the field, we hope it is clear that MetSim really does not aim to implement any new methods, but simply improve the infrastructure of some of the common methods that already existed in ad-hoc frameworks. |
This comment has been minimized.
This comment has been minimized.
Hey @arbennett! Thanks for your quick actions!
Can you point me to the URL? I could not find it, neither in the installation instructions in the docs nor in the README
Thanks for the clarification! Otherwise, only the part about State of the field is unclear for me (see my comment above @sjpfenninger) |
This comment has been minimized.
This comment has been minimized.
@Chilipp I updated the readme so that the line containing |
This comment has been minimized.
This comment has been minimized.
Ha, thanks @arbennett. Sorry, I must have overlooked that line |
This comment has been minimized.
This comment has been minimized.
@Chillipp thanks for the review and @arbennett thanks for addressing them so rapidly! @arbennett can you make sure to address the point raised about covering State of the field -- i.e., referring to other published packages that MetSim is comparable, complements, or goes beyond? From your response above it sounds like this kind of code is usually set up on an ad-hoc basis and there are no published packages - if so, I would also point that out in the paper. |
This comment has been minimized.
This comment has been minimized.
@dsryberg looking forward to your review as well! |
This comment has been minimized.
This comment has been minimized.
Thanks @sjpfenninger - I have updated the text to include a paragraph discussing the state of the field! |
This comment has been minimized.
This comment has been minimized.
Thanks @arbennett! For me, this paper is ready for publication @sjpfenninger |
This comment has been minimized.
This comment has been minimized.
Hi all! Sorry for the delay/confusion on my part. I have now also completed the review, but I am unable to adjust the checklist since I cannot complete this step:
I am told that the invitation has expired. Is this easy to fix? |
This comment has been minimized.
This comment has been minimized.
I've just re-invited you. The link at https://github.com/openjournals/joss-reviews/invitations should work again now. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Unfortunately I am still told "Sorry, we couldn't find that repository invitation. It is possible that the invitation was revoked or that you are not logged into the invited account." Although I have also made sure I am logging into the account by trying this on three different devices |
This comment has been minimized.
This comment has been minimized.
Can you try again right now @dsryberg? |
This comment has been minimized.
This comment has been minimized.
Everything seems to be working now. I will input my comments now... |
This comment has been minimized.
This comment has been minimized.
Overall: I agree that MetSim is a well structured, well documented, and sufficiently tested package, and I would definitely support it being published via JOSS. I nevertheless had a few hiccups during the review/testing process, and I also have trouble understanding who are the intended users of MetSim and what issue it is meant to resolve. I also suggest reworking some of the example scripts, as aspects of these failed for me. Finally, I think more detail should be given regarding MetSim's "Disaggregation of daily simulation values to sub-daily timesteps". I'll go into more detail below: Functionality
Documentation
Software paper
Other thoughts
|
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@dsryberg thanks for the thorough review! I have updated the text of the paper as well as the documentation here to reflect the algorithms used to disaggregate. The more recent version of the article proof contains additional text about the state of the field, particularly with respect to MTCLIM and VIC. Regarding your point number 1, I want to be clear that we aren't trying to say that MetSim is preferable to something like ERA5, but is complementary. MetSim can be used to generate sub-daily timeseries when something like ERA5 cannot be used (pre-1950, smaller spatial scales, etc). This actually brings up your point 3 as well. MetSim can be used at any spatial scale or mesh arrangement. I have also added a sentence to the paper to address this. I have also updated the tutorial setup so that the Pandas error no longer occurs. |
This comment has been minimized.
This comment has been minimized.
OK. v2.2.1 is the version. |
This comment has been minimized.
This comment has been minimized.
@whedon check references |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@arbennett Almost there, thanks for your patience. I have updated the version to v2.2.1. I've also noticed a couple small issues with the bibliography:
Then, please 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! |
This comment has been minimized.
This comment has been minimized.
@sjpfenninger - thanks again for your efforts in editing this paper. I've updated the DOIs for the dask and pandas citations. I've also uploaded to Zenodo. The DOI is 10.5281/zenodo.3728015 |
This comment has been minimized.
This comment has been minimized.
@whedon set 10.5281/zenodo.3728015 as archive |
This comment has been minimized.
This comment has been minimized.
OK. 10.5281/zenodo.3728015 is the archive. |
This comment has been minimized.
This comment has been minimized.
@whedon accept |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Check final proof If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1390, then you can now move forward with accepting the submission by compiling again with the flag
|
This comment has been minimized.
This comment has been minimized.
@sjpfenninger - this is ready to go? All final proofreading is done? |
This comment has been minimized.
This comment has been minimized.
@arbennett - I've suggested some changes in UW-Hydro/MetSim#216 and UW-Hydro/MetSim#217 |
This comment has been minimized.
This comment has been minimized.
@danielskatz I was going to say yes, but it looks like you found a couple of remaining issues. Other than those, it's ready to go as far as I'm concerned! |
This comment has been minimized.
This comment has been minimized.
@danielskatz - thanks for the suggestions, I've merged the pull requests! |
This comment has been minimized.
This comment has been minimized.
@arbennett - you merged one, but the other is just approved but not yet merged... |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@whedon accept deposit=true |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Here's what you must now do:
Any issues? notify your editorial technical team... |
This comment has been minimized.
This comment has been minimized.
Thanks to @Chilipp & @dsryberg for reviewing! Congratulations to @arbennett and co-authors! |
This comment has been minimized.
This comment has been minimized.
If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
whedon commentedJan 27, 2020
•
edited
Submitting author: @arbennett (Andrew Bennett)
Repository: https://github.com/UW-Hydro/MetSim
Version: v2.2.1
Editor: @sjpfenninger
Reviewer: @Chilipp, @dsryberg
Archive: 10.5281/zenodo.3728015
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
@Chilipp & @dsryberg, 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.
Review checklist for @Chilipp
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @dsryberg
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper