Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW]: MetSim: A Python package for estimation anddisaggregation of meteorological data #2042

Closed
37 of 38 tasks
whedon opened this issue Jan 27, 2020 · 69 comments
Closed
37 of 38 tasks
Assignees
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Jan 27, 2020

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

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/c232fbc73fc2ad0c3a2e8a4c9e318236"><img src="https://joss.theoj.org/papers/c232fbc73fc2ad0c3a2e8a4c9e318236/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/c232fbc73fc2ad0c3a2e8a4c9e318236/status.svg)](https://joss.theoj.org/papers/c232fbc73fc2ad0c3a2e8a4c9e318236)

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:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

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 @Chilipp

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@arbennett) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • 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?

Review checklist for @dsryberg

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@arbennett) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • 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?
@whedon
Copy link
Author

whedon commented Jan 27, 2020

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 🎉.

⭐ Important ⭐

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:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon
Copy link
Author

whedon commented Jan 27, 2020

Reference check summary:

OK DOIs

- 10.1145/2833157.2833162 is OK
- 10.5334/jors.148 is OK
- 10.1016/0266-9838(86)90020-1 is OK
- 10.1016/S0168-1923(98)00126-9 is OK
- 10.1016/j.agrformet.2013.03.003 is OK
- 10.1175/JHM-D-18-0203.1 is OK
- 10.1029/94JD00483 is OK

MISSING DOIs

- https://doi.org/10.25080/majora-7b98e3ed-013 may be missing for title:  Dask: Parallel Computation with Blocked algorithms and Task Scheduling 

INVALID DOIs

- None

@whedon
Copy link
Author

whedon commented Jan 27, 2020

@Chilipp
Copy link

Chilipp commented Jan 27, 2020

Thanks @sjpfenninger! I am busy this week with other appointments, but I will do the review on monday in a week.

@Chilipp
Copy link

Chilipp commented Feb 7, 2020

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:


  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

In the paper you state:

building upon previous tools by improving performance

In what sense does MetSim improve the performance compared to the previous tools?


  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

The documentation of the metsim.methods.mtclim module is missing (see UW-Hydro/MetSim#202). The same is true for the metsim.physics.solar_geom function.


  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

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).


  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support
    I could not find them. Please add a CONTRIBUTING file or something equivalent.

  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
    I am not sure @sjpfenninger what is required here for the software paper as the discipline is quite diverse. The authors state in the paper

We have based MetSim on methods from the Mountain Microclimate Simulator (MTCLIM) and the forcing preprocessor that was built into the Variable Infiltration Capacity (VIC) hydrological model version 4.
MetSim provides a modern workflow, building upon previous tools by improving performance, adding new IO routines, allowing for exact restarts, and providing an extensible architecture which can incorporate new features.
We have designed MetSim to fit into the broader scientific Python ecosystem, building on popular packages such as xarray, dask, pandas, and numba.

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).


@arbennett
Copy link

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.

@Chilipp
Copy link

Chilipp commented Feb 11, 2020

Hey @arbennett! Thanks for your quick actions!

Regarding tests: we do show how to run the tests when installing from source

Can you point me to the URL? I could not find it, neither in the installation instructions in the docs nor in the README

I added a bit to the performance claim, but the main takeaway is that we have implemented scalable parallelism

Thanks for the clarification!

Otherwise, only the part about State of the field is unclear for me (see my comment above @sjpfenninger)

@arbennett
Copy link

@Chilipp I updated the readme so that the line containing pytest --verbose is a little bit more clearly highlighted.

@Chilipp
Copy link

Chilipp commented Feb 11, 2020

Ha, thanks @arbennett. Sorry, I must have overlooked that line 😅

@sjpfenninger
Copy link

@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.

@sjpfenninger
Copy link

@dsryberg looking forward to your review as well!

@arbennett
Copy link

Thanks @sjpfenninger - I have updated the text to include a paragraph discussing the state of the field!

@Chilipp
Copy link

Chilipp commented Feb 21, 2020

Thanks @arbennett! For me, this paper is ready for publication @sjpfenninger

@sevberg
Copy link

sevberg commented Mar 5, 2020

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:

Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

I am told that the invitation has expired. Is this easy to fix?

@arfon
Copy link
Member

arfon commented Mar 5, 2020

I am told that the invitation has expired. Is this easy to fix?

I've just re-invited you. The link at https://github.com/openjournals/joss-reviews/invitations should work again now.

@sjpfenninger
Copy link

Thanks @arfon for the quick reaction and no worries @dsryberg, looking forward to your review!

@sevberg
Copy link

sevberg commented Mar 6, 2020

I am told that the invitation has expired. Is this easy to fix?

I've just re-invited you. The link at https://github.com/openjournals/joss-reviews/invitations should work again now.

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

@arfon
Copy link
Member

arfon commented Mar 6, 2020

Can you try again right now @dsryberg?

Screen Shot 2020-03-06 at 8 07 18 AM

@sevberg
Copy link

sevberg commented Mar 6, 2020

Everything seems to be working now. I will input my comments now...

@sevberg
Copy link

sevberg commented Mar 6, 2020

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

  • Installation: Does installation proceed as outlined in the documentation?
  • Installation was no issue for me on both a Linux and Windows PC. Although, as a minor issue, you may want to complete the same list of packages seen in the README as in the environment.yml file.
  • Functionality: Have the functional claims of the software been confirmed?
  1. I successfully ran the built-in tests, and all 9 passed. This is great, but, executing these tests took 11 minutes, which is in my opinion long considering that they should be run often during future development processes. Especially the tests which run the examples "example_nc.conf" and "constant_vars_nc.conf" took the most time, so perhaps these could be simplified? In any case, 6 of the 9 tests are simply running the examples and they only ensure that an output file has been created. This will catch any raised errors, but it will miss the more subtle "sneaky" errors. Therefore I would suggest improving these by also checking some of the values which are produced to ensure that the code is working as intended.

    • Additionally, when I ran the tests I also evaluated the code coverage, which came out to a total of 75%, which is quite good! Nevertheless, the code within the file "physics.py" had the lowest specific score of 34%. Since the functions in this file are integral to the operation of MetSim, perhaps additional tests could be added to increase the coverage here.
  2. I then tried to follow the tutorials found at MetSim-tutorial

    1. Example 1 worked as expected
    2. Example 2 appeared to work but...
      • Interactive plots were not functional (likely an issue unrelated to MetSim)
      • Tried changing timestep to 15 minutes
        • MetSim appeared to work as intended
        • There seems to be a bug in the way timestamps are saved. This leads to an error when plotting in the example. It is unclear how to resolve this issue
    3. Example 3 failed due to an issue with pandas. Maybe a result of conda installing the newest pandas version (1.0)
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)
  • From my perspective, MetSim does not seem to consume an undue amount of resources and parallelizes well. This is also discussed sufficiently in the paper.

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • (Good) Statements are given in the README which relate to what MetSim is capable of
  • (Missing) The existing problem which the software solves is not specifically stated in the README
  • (Missing) The intended audience is also not specified either
  • (Concern) I am not a part of the climate modelling community (so I may be missing context which is otherwise obvious), however I do not understand the need to a climate data disagregator which operates on daily data when sub-daily data is already readily available from many sources (see MERRA2, ERA5, ect...). It is not clear to me who is meant to benefit from this software.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • The examples are included and furthermore seem quite detailed but they are in my opinion cryptic. Instead of beginning the examples with "# This is an example of an input file for MetSim", I would suggest an introductory description of what the example is meant to accomplish.
  • Also, please see the above comment regarding providing more examples/tests which cover the functions in "physics.py"
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • The documentation is very well done. Although I've noticed differences between the documentation available on readthedocs versus what I can build from the cloned repository. For example, when I build the docs from the repository (using simply "make html") I do not see any items under the "API Reference" section. Although I'm not sure if this issue was caused by the doc files in MetSim, or else by Sphinx.
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Yes, but see the notes above concerning Functionality

Software paper

  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • I agree with the comment from @Chilipp that the state of the field is not well addressed. I also agree with @sjpfenninger, that, if there are not other modules to compare, against then this can be pointed out instead.

Other thoughts

  1. It should be stated why using MetSim to disaggregate daily weather variables is preferable to simply downloading and using RCM or GCM products which are already hourly (for example, MERRA, ERA5, SARAH, ect...)
  2. When listing the disaggregation capabilities of MetSim in the README, a brief note should be given regarding the quality or source of the algorithms used. For example, there is a sophisticated algorithm behind the solar irradiance disaggregation, but the wind speed disaggregation is just a forward fill for each time step in the day (at least, as far as I can tell by snooping through the code). An unaware user might not understand this disparity.
  3. I also have not found any information regarding spatial resolution. Are the models implemented in MetSim intended to be applicable on all spatial scales?

@arbennett
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Mar 6, 2020

@arbennett
Copy link

@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.

@sjpfenninger
Copy link

@whedon set 10.5281/zenodo.3728015 as archive

@whedon
Copy link
Author

whedon commented Mar 27, 2020

OK. 10.5281/zenodo.3728015 is the archive.

@sjpfenninger
Copy link

@whedon accept

@whedon
Copy link
Author

whedon commented Mar 27, 2020

Attempting dry run of processing paper acceptance...

@whedon whedon added the recommend-accept Papers recommended for acceptance in JOSS. label Mar 27, 2020
@whedon
Copy link
Author

whedon commented Mar 27, 2020

Reference check summary:

OK DOIs

-  10.25080/Majora-7b98e3ed-013  is OK
-   10.25080/Majora-92bf1922-00a  is OK
- 10.1145/2833157.2833162 is OK
- 10.5334/jors.148 is OK
- 10.1016/0266-9838(86)90020-1 is OK
- 10.1016/S0168-1923(98)00126-9 is OK
- 10.1016/j.agrformet.2013.03.003 is OK
- 10.1175/JHM-D-18-0203.1 is OK
- 10.1029/94JD00483 is OK

MISSING DOIs

- None

INVALID DOIs

- None

@whedon
Copy link
Author

whedon commented Mar 27, 2020

👋 @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof 👉 openjournals/joss-papers#1390

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 deposit=true e.g.

@whedon accept deposit=true

@danielskatz
Copy link

@sjpfenninger - this is ready to go? All final proofreading is done?

@danielskatz
Copy link

@arbennett - I've suggested some changes in UW-Hydro/MetSim#216 and UW-Hydro/MetSim#217

@sjpfenninger
Copy link

@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!

@arbennett
Copy link

@danielskatz - thanks for the suggestions, I've merged the pull requests!

@danielskatz
Copy link

@arbennett - you merged one, but the other is just approved but not yet merged...

@arbennett
Copy link

🤦‍♂️ - sorry, I have merged it now!

@danielskatz
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Mar 27, 2020

@danielskatz
Copy link

@whedon accept deposit=true

@whedon
Copy link
Author

whedon commented Mar 27, 2020

Doing it live! Attempting automated processing of paper acceptance...

@whedon whedon added accepted published Papers published in JOSS labels Mar 27, 2020
@whedon
Copy link
Author

whedon commented Mar 27, 2020

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

@whedon
Copy link
Author

whedon commented Mar 27, 2020

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 Creating pull request for 10.21105.joss.02042 joss-papers#1391
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.02042
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

@danielskatz
Copy link

Thanks to @Chilipp & @dsryberg for reviewing!
And to @sjpfenninger for editing!

Congratulations to @arbennett and co-authors!

@whedon
Copy link
Author

whedon commented Mar 27, 2020

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.02042/status.svg)](https://doi.org/10.21105/joss.02042)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.02042">
  <img src="https://joss.theoj.org/papers/10.21105/joss.02042/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.02042/status.svg
   :target: https://doi.org/10.21105/joss.02042

This is how it will look in your documentation:

DOI

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

7 participants