[REVIEW]: diyepw: A Python package for Do-It-Yourself EnergyPlus 1 weather file generation

whedon commented 3 years ago

Submitting author: @amandadsmith (Amanda D. Smith) Repository: https://github.com/IMMM-SFA/diyepw/ Version: v1.2.1 Editor: @timtroendle Reviewers: @samuelduchesne, @fneum Archive: 10.5281/zenodo.5258122

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

Status badge code:

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

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

@samuelduchesne & @ritwikagarwal, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
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 @timtroendle know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @samuelduchesne

Conflict of interest

[x] 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

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@amandadsmith) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

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

Conflict of interest

[x] 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

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@amandadsmith) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

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

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @samuelduchesne, @ritwikagarwal it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

: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:

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

watching

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 commented 3 years ago

PDF failed to compile for issue #3313 with the following error:

 Can't find any papers to compile :-(

whedon commented 3 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.06 s (554.3 files/s, 42556.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          20            314            594           1114
reStructuredText                 3             65             45            122
Markdown                         6             23              0            119
DOS Batch                        1              8              1             26
make                             1              4              7              9
TOML                             1              0              0              6
-------------------------------------------------------------------------------
SUM:                            32            414            647           1396
-------------------------------------------------------------------------------

Statistical information for the repository 'e59152558c545a9968fdc443' was
gathered on 2021/05/26.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Amanda Smith                     7           738              8            2.06
Benjamin Stürmer               157          4175           3004           19.85
travis                           8         14157          14091           78.09

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Benjamin Stürmer           1944           46.6          3.7               15.33
travis                       78            0.6          1.1               34.62

timtroendle commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

timtroendle commented 3 years ago

@samuelduchesne, @ritwikagarwal, we are still in reduced service mode in which we ask reviewers to finish their reviews within six weeks. I will add an automatic reminder for each of you to indicate when half of that time has elapsed.

Of course it's great if you can finish your review earlier than that.

timtroendle commented 3 years ago

@whedon remind @samuelduchesne in three weeks

whedon commented 3 years ago

Reminder set for @samuelduchesne in three weeks

timtroendle commented 3 years ago

@whedon remind @ritwikagarwal in three weeks

whedon commented 3 years ago

Reminder set for @ritwikagarwal in three weeks

whedon commented 3 years ago

:wave: @samuelduchesne, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @ritwikagarwal, please update us on how your review is going (this is an automated reminder).

samuelduchesne commented 3 years ago

Hi @amandadsmith, just had a chance to go trough this. A tool to automatically generate weather files from NOAA data is very useful! I must say that this submission is of very good quality and it shows the care you and your team put to provide an open-source tool to the building energy modeling community.

I the spirit of making the tool even better I have some recommendations that I invite your to consider even if they would not block the acceptance of this submission.

Continuous Integration

Although the package has automated testing, it is not part of a CI tool such as GitHub Actions, Travis CI or Circle CI. Setting up continuous integration with any of these services is very easy and will save you a lot of time (and headaches) when publishing new releases of the tool in the future. You could even test different platforms (linux, windows and mac simultaneously). My recommendation: get started right away with Github Actions.

Command Line Interface

I do not agree with issue #31! Having 2 separate repositories, one for your python API and another for your CLI interface is cumbersome and some users will hate your for it. You could rather include all the cli functions as an actual shell program using Click. Then users simply have to pip install diyepw and in the same shell, they can run diyepw analyze_noaa_data --help to get something similar to this:

$ dryepw analyze_noaa_data --help
Usage: analyze_noaa_data [OPTIONS]

  Simple program that analyzes..

Options:
  --option_1 INTEGER 
  --option_2 TEXT
  --option_3

In conclusion, I recommend the publication of this submission as is, but I strongly recommend that the authors connect the repo with a CI tool and that they merge the scripts within the same repository and implement a CLI using click.

amandadsmith commented 3 years ago

Hi @samuelduchesne, we greatly appreciate your thoughtful review! These suggestions are valuable and we plan to implement. Thanks for pointing out the Click package.

whedon commented 3 years ago

:wave: @ritwikagarwal, please update us on how your review is going (this is an automated reminder).

timtroendle commented 3 years ago

Hello @ritwikagarwal, we are slowly approaching the end of the six-week review period. Can you let us know where you stand with your review?

timtroendle commented 3 years ago

Hi @amandadsmith, to keep you up to date: I am currently looking for a second reviewer and a second review of your submission. I will update you as soon as I made progress in my search.

amandadsmith commented 3 years ago

Thank you for your efforts @timtroendle

timtroendle commented 3 years ago

Apologies for the slow process, @amandadsmith . It's a bit difficult to find a replacement reviewer at this point, as many people are about to leave or have left into their summer breaks. I found a few people that would like to review this submission but can do so only in a few days time. I will assign one of them as soon as they are ready. Please bear with us.

amandadsmith commented 3 years ago

I understand @timtroendle. It's a hard time to reach academics. Thanks for keeping us apprised of what's going on.

timtroendle commented 3 years ago

@whedon remove @ritwikagarwal as reviewer

whedon commented 3 years ago

OK, @ritwikagarwal is no longer a reviewer

timtroendle commented 3 years ago

@whedon add @fneum as reviewer

whedon commented 3 years ago

OK, @fneum is now a reviewer

timtroendle commented 3 years ago

@amandadsmith , we have a new reviewer for your submission. I am sorry for the delay.

:wave: @fneum, thanks a lot for offering to review this submission.

fneum commented 3 years ago

Hi @amandadsmith, I have just finished my review of your diyepw package. I very much like and support the idea to build a tool that automates the pipeline from historical weather data to uses in building energy modelling. In this case, from NOAA data to EPW files. Thanks for the extra effort you go through in making this open-source and accessible to others! I also appreciate the functionality to use heuristics to fill gaps in the data as well as the detailed tutorial.

For the record, I second both of @samuelduchesne's comments regarding continuous integration and keeping the CLI in the same repository. Great that these suggestions have both been implemented already!

I also have a few suggestions relating to a few errors I encountered while testing (PyPI packaging might be the main culprit here) and extending the introduction of the documentation (mostly using the work you put into the paper). I hope you will find the review useful!

Functionality

[x] diyepw.get_wmo_station_location(725300) results in an error
```
Exception: Missing WMO station info file at /home/USER/miniconda3/envs/diyepw/lib/python3.9/site-packages/diyepw/data/Weather_Stations_by_County.csv
```
I have used pip to install diyepw from PyPI. Do you ship the data folder with the pip installation? I think the paths in the package data might not be up to date (https://github.com/IMMM-SFA/diyepw/blob/63e8e9ad4d3a8a9490f2eb2509caa2afa7d328f6/setup.py#L25). It works if I install directly from the repository with pip install -e .. Could you check that?
[x] Is it intentional to not implement the defaults mentioned in https://diyepw.readthedocs.io/en/latest/README.html#using-diyepw-to-generate-amy-epw-files for max_records_to_interpolate and max_records_to_impute in the function create_amy_epw_file and similarly for create_amy_epw_files_for_years_and_wmos?

[x] When running the CLI, e.g. create_amy_epw_files_for_years_and_wmos --help, I get an error:

Traceback (most recent call last):
File "/home/ws/sp2668/miniconda3/envs/diyepw/bin/create_amy_epw_files_for_years_and_wmos", line 5, in <module>
from diyepw.scripts.create_amy_epw_files_for_years_and_wmos import create_amy_epw_files_for_years_and_wmos
ModuleNotFoundError: No module named 'diyepw.scripts'

Do you know what I might be doing wrong here, or whether this is a bug?

[x] The jupyter notebook autocomplete shift + TAB only shows the parameters, not the corresponding explanations. It would be nice to have them show up there. Would that be simple to fix? (minor)
[x] Any reason why analyze_noaa_isd_lite_files has compulsory arguments, but analyze_noaa_isd_lite_file does not? I would have expected both to function widely analogously. (minor)
[x] The versions in setup.py are mostly fixed. Any chance you can loosen these constraints? (minor)

Documentation

[x] The first paragraph of the README/documentation directly uses the abbreviations EPW and WMO. For the latter, could you mention what the abbreviation stands for, for the former, could you link to an explanation of the file format and add some background where these files are commonly used?
[x] In general, the introduction is quite minimal. Sure, it doesn't need to be long, but given the documentation/README will be the primary point of entry for prospective users, I think it would be helpful to copy over some of the background, context, statement of need from the paper.
[x] Particularly, the data source NOAA ISD Lite could be featured more prominently.
[x] Could you link to a website that lists the available codes for the WMOs? Actually, it would be useful to have such a reference also in the docstring of functions like create_amy_epw_files_for_years_and_wmos. I found a link in the tutorial, but currently it's somewhat hidden.
[x] Please also add a page like "Contributing" with community guidelines (one of the boxes I need to tick above)
[x] You could link to the documentation in the About section of the Github repository (top right) (minor)

Paper

[x] L21: "Its output is a weather file"
[x] L25-26: The separation of diyepw and diyepw-scripts seems to be outdated: https://github.com/IMMM-SFA/diyepw/issues/51
[x] L51f: Could you add another sentence on the functionality/background of LAF (apart from the limitations)?
[x] references: many links are duplicated, could you remove the duplicates?
[x] references: could you round out the citations of numpy, pandas, xarray? I.e. with DOI following instructions at https://numpy.org/citing-numpy/, https://pandas.pydata.org/about/citing.html, http://xarray.pydata.org/en/stable/getting-started-guide/faq.html#how-should-i-cite-xarray

timtroendle commented 3 years ago

/ooo September 4 until September 26

timtroendle commented 3 years ago

@amandadsmith, I will be out of office for the duration mentioned above. Do you think you will be able to handle @fneum's comments before that?

amandadsmith commented 3 years ago

@fneum Thank you for the thorough review, and providing a checklist. Our team is working through these items now.

@timtroendle Yes, I think so. Thank you for the heads up and the reminder.

thurber commented 3 years ago

Hi @fneum, thanks for reporting these issues! We've released diyepw v1.2.1 to PyPi that should address these points:

diyepw.get_wmo_station_location(725300) should be fixed - yes, this was issue in the setup.py with pathfinding.
Implemented the defaults in most of the methods.
Hopefully fixed the CLI issues -- if you're still seeing this can you let me know what OS you're running on?
Jupyter autocomplete -- we were missing the :param: documentation in some of the methods, so we've added this in
For analyze_noaa_isd_lite_file(s) methods, we added the default parameters to the plural version, but decided to still leave them out of the singular version in case consuming workflows want to make their own decisions about that.
Updated documentation based on your feedback (added information and links about EPW, WMO, and ISD, as well as a CONTRIBUTING.md section)

Regarding fixed dependency versions in setup.py -- this is understandably a pain point, but we find it important to pin the versions in order to favor full reproducibility for any experiments making use of the code. diyepw will most likely still work with alternate dependency versions, but you'll need to manually install in that case. We recommend maintaining a unique virtual environment for diyepw workflows.

amandadsmith commented 3 years ago

HI @fneum we really appreciate your support and your suggestions here. I'll skip the Functionality items as I believe @thurber has touched on all of them above. I believe we've gotten each of the Documentation/Paper issues, and I think it's more clear and up to date, so thanks again for your thoroughness!

Documentation

[x] The first paragraph of the README/documentation directly uses the abbreviations EPW and WMO. For the latter, could you mention what the abbreviation stands for, for the former, could you link to an explanation of the file format and add some background where these files are commonly used?

[x] In general, the introduction is quite minimal. Sure, it doesn't need to be long, but given the documentation/README will be the primary point of entry for prospective users, I think it would be helpful to copy over some of the background, context, statement of need from the paper.

We've added a little material here but will also link directly from this page to the paper when published.

[x] Particularly, the data source NOAA ISD Lite could be featured more prominently.

[x] Could you link to a website that lists the available codes for the WMOs? Actually, it would be useful to have such a reference also in the docstring of functions like create_amy_epw_files_for_years_and_wmos. I found a link in the tutorial, but currently it's somewhat hidden.

[x] Please also add a page like "Contributing" with community guidelines (one of the boxes I need to tick above)

[x] You could link to the documentation in the About section of the Github repository (top right) (minor)

Paper

[x] L21: "Its output is a weather file"

[x] L25-26: The separation of diyepw and diyepw-scripts seems to be outdated: Implement cli within main repo and remove the diyepw-scripts repo IMMM-SFA/diyepw#51

[x] L51f: Could you add another sentence on the functionality/background of LAF (apart from the limitations)?

[x] references: many links are duplicated, could you remove the duplicates?

[x] references: could you round out the citations of numpy, pandas, xarray? I.e. with DOI following instructions at https://numpy.org/citing-numpy/, https://pandas.pydata.org/about/citing.html, http://xarray.pydata.org/en/stable/getting-started-guide/faq.html#how-should-i-cite-xarray

fneum commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

fneum commented 3 years ago

Thanks @thurber and @amandadsmith for working through the feedback. All changes look good to me and all boxes are ticked now. No worries regarding the fixed versions. You clearly thought about this already.

timtroendle commented 3 years ago

@whedon check references on branch joss-paper

timtroendle commented 3 years ago

@whedon check references from branch joss-paper

whedon commented 3 years ago

Attempting to check references... from custom branch joss-paper

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.softx.2019.100299 is OK
- 10.1016/j.apenergy.2017.08.244 is OK
- 10.1175/2011BAMS3015.1 is OK
- 10.5334/jors.148 is OK
- 10.5281/zenodo.4299126 is OK
-  10.25080/Majora-92bf1922-00a  is OK
- 10.5281/zenodo.5203279 is OK
- 10.1038/s41586-020-2649-2 is OK

MISSING DOIs

- None

INVALID DOIs

- None

timtroendle commented 3 years ago

Thank you @fneum for your review.

@amandadsmith, it all looks good to me. Is v1.2.1 the correct version? At this point, could you please

[ ] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)
[ ] Check the archival deposit (e.g., in Zenodo) has the correct metadata. This includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it). You may also add the authors' ORCID.
[ ] Please list the DOI of the archived version here.

I can then move forward with accepting the submission.

thurber commented 3 years ago

@timtroendle, here's the DOI for v1.2.1: 10.5281/zenodo.5258122

amandadsmith commented 3 years ago

Thank you @fneum for your review.

@amandadsmith, it all looks good to me. Is v1.2.1 the correct version? At this point, could you please

[x] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)

https://github.com/openjournals/joss-reviews/issues/3313#issuecomment-907672011

[x] Check the archival deposit (e.g., in Zenodo) has the correct metadata. This includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it). You may also add the authors' ORCID.

I confirm the title matches, author list is correct and includes ORCIDs.

[x] Please list the DOI of the archived version here.

https://github.com/openjournals/joss-reviews/issues/3313#issuecomment-907672011

I can then move forward with accepting the submission.

Thank you!

timtroendle commented 3 years ago

@whedon set v1.2.1 as version

whedon commented 3 years ago

OK. v1.2.1 is the version.

timtroendle commented 3 years ago

@whedon set 10.5281/zenodo.5258122 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.5258122 is the archive.

timtroendle commented 3 years ago

@amandadsmith, @thurber , thanks! I will recommend to accept your submission. Many thanks again for your thorough reviews, @samuelduchesne and @fneum!

timtroendle commented 3 years ago

@whedon recommend-accept

whedon commented 3 years ago

Attempting dry run of processing paper acceptance...

openjournals / joss-reviews