-
-
Notifications
You must be signed in to change notification settings - Fork 38
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]: epwshiftr: Create future EnergyPlus Weather files using CMIP6 data #4030
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @mitmat, @bczernecki it looks like you're currently assigned to review this paper 🎉. 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. ⭐ 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:
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:
|
Failed to discover a |
Wordcount for |
|
|
Hi @hongyuanjia, you might want to review the "What should my paper contain?" section of the JOSS author guide to ensure your paper has the required sections in it. |
Hi @KristinaRiemer, thanks for the remainder. I will update the paper structure accordingly based on the guide. |
Hi @hongyuanjia, I started to review the package. I got to test the functionality until |
Hi @mitmat, thanks for the review. EnergyPlus is an open-source building energy simulation. It provides free available worldwide weather data at https://energyplus.net/weather. The one I used in the example can be downloaded at the EnergyPlus GitHub repo: https://raw.githubusercontent.com/NREL/EnergyPlus/develop/weather/USA_CA_San.Francisco.Intl.AP.724940_TMY3.epw. Please let me know if you encounter any issue about accessing the data. |
I got through the examples in the repo readme. Package works like a charm. Already the first part of managing the many files is solved nicely. I have no real issues with the functionality, but the documentation needs to be improved! (see also the issues in the repo). Comments to the paper will come later… License: You have two license files in the repo, the one is MIT (as specified on CRAN, too), and the other is a personal copyright (non-permissive, I guess?). For JOSS, an open license is required, so please check this. Contribution and authorship: From the github commit history, it’s clear that the main author has made major contributions to the package. The co-author’s contribution is not clear: his extremely few commits are not of substantial nature, so please clarify the author list and contributions. Functionality While using the package, one likely will have to work with many files. I like the approach you’ve taken to manage a file index. However, I suggest to explain better how you set the custom directory, and maybe give some hints on why one should do this. In What is How do you deal with non-standard calendars in GCMs? Noleap or 360? Documentation The BTW, out of curiosity, are you aware of any packages in R that query ESGF servers? And if yes, do they have the option of login? I know of something in python, but have not seen anything for R yet. |
👋 @mitmat, please update us on how your review is going (this is an automated reminder). |
👋 @bczernecki, please update us on how your review is going (this is an automated reminder). |
I finished the review. Please find in a post above the comments to the package, and some new issues in the pkg repo. As addon to above: The function For the morphing methods, or generally when working with climate data, one needs to use climatological averages (i.e. 30 year averages). Maybe you could give the users a warning if they fail to do so, when calling the morphing function? Finally, regarding the paper: The package is nice in a way that it provides the full process from managing climate model data, performing bias adjustment, and giving output in the desired format. Section on “Statement of need” is missing I wonder how it compares to existing packages, which of course do not have all the functionality, but maybe parts? Is there anything in R for manging CMIP6 (or CMIP5, for that matter), or other ESGF data? Bias adjustment and downscaling is an active topic, I’m sure some packages exist? (-> missing “state of the field”) Some disclaimer on using climate data would be nice (maybe also in the github repo). It’s great that the climate data is freely available, but still users need to be made aware of what climate model data can or cannot do. See e.g. the well-made guide from EURO-CORDEX https://www.euro-cordex.net/imperia/md/content/csc/cordex/guidance_for_euro-cordex_climate_projections_data_use__2021-02_1_.pdf The term morphing is typical for building simulations, but rarely used outside of the field. You might want to mention the term “bias adjustment” or “downscaling”, which are commonly used. https://hypeweb.smhi.se/what-is-bias-adjustment/ Minor comments: L13: GCM usually refers to General Circulation Model, not Global Climate model And the last thing, out of personal curiosity, not necessarily relevant for the review: |
Thank you @mitmat for taking the time to give such thorough reviews of epwshiftr! I will start to address all your comments and issues opened in the epwshiftr repo in the following days.
epwshiftr is released under MIT license. Unlike GPL license, MIT license is a template that requires additional details to be complete in the As described in https://r-pkgs.org/license.html#key-files, it is common practice to include a copy of the license file in open-source software. Since CRAN does not permit to include a copy of standard licenses in packages, the full copy of the license is included in
The epwshiftr was developed during my postdoc and was supported by a research project of Adrian, the co-author. Even Adrian did not commit to the repo that much, but he contributed a lot to the development underneath via conceptualization and supervision through lots of regular discussions on project direction and design.
Thanks for the suggestion. The user can use the option
This relates to the way that epwshiftr handles datetime data in NetCDF files, as you also mentioned in the following question:
Most of output NetCDFs from CMIP6 GCMs use Proleptic Gregorian calendar. epwshiftr uses Why I need the previous and next year is that, taking
|
It looks like @hongyuanjia has started to respond to @mitmat's comments, thank you for your thoroughness with those! And I agree with the conclusion about authorship; intellectual contributions are more than sufficient for authorship, see the JOSS authorship guidelines. @bczernecki it looks like you've started your review, how is it going? |
I have quite thoroughly investigated content of the Most of my concerns and comments stay in line with everything that was previously suggested by @mitmat, i.e.
Other comments are related to the problems that I found while working with the examplary workflow suggested by README file:
I think developer should write a solution that basically checks whether downloading in R is possible and if not then provide message or even recommend external packages for this purposes that can be implemented with epwshiftr. Moreover, it is not clear to me where this NetCDF files should be downloaded (into working directory or tmp folder that was created before?). I could find it in the next step, but would be good to know it in advance
|
@hongyuanjia have you had a chance to look over @bczernecki's comments yet? @bczernecki, with regards to coauthorship, our JOSS authorship guidelines err on the side of letting the authors decide who has made meaningful enough non-code contributions to warrant authorship. It sounds like those contributions were sufficient in this case. About the test coverage, maybe it would be more useful to determine if the current set of tests cover the most important or error-prone functionality of the software, as opposed to just a threshold for test coverage percent? Are there parts of this code that currently lack tests but could really benefit from them? |
@KristinaRiemer Sorry for the late. I just came back to work from the Chinese New Year holiday. I will start to address @bczernecki comments in the following week. LicenseThank you @KristinaRiemer for further elaboration on the coauthorship requirement of JOSS. Unit testsActually, the test coverage of epwshiftr is 100%. The drop of coverage to ~63% is due to the undesired test execution order. Please see ideas-lab-nus/epwshiftr#35. This has been fixed in ideas-lab-nus/epwshiftr#36. Available GCMs (
|
Hi @hongyuanjia! How is your progress on the list of issues going? Do you need anything to help facilitate right now? |
Hello @hongyuanjia, how are the improvements on your submission going? |
@KristinaRiemer Sorry for the late. Unfortunately, I am slowly moving forward. Right now I mainly focus on addressing ideas-lab-nus/epwshiftr#40. Will address reviewer comments related to features (ideas-lab-nus/epwshiftr#32, ideas-lab-nus/epwshiftr#19, ideas-lab-nus/epwshiftr#43) and bugs (ideas-lab-nus/epwshiftr#39) during this weekend. I will try my best to solve documentation-related issues next week. |
@hongyuanjia it's all good! I just wanted to make sure things are moving along and nothing is blocking your work. |
Hi @hongyuanjia, how are your updates to this software progressing? It looks like you have closed some of the issues. |
Hi @KristinaRiemer. Most of the feature-related issues have been solved, except for ideas-lab-nus/epwshiftr#19, which may need more refactoring. Also, to solve ideas-lab-nus/epwshiftr#38 in a more comprehensive way, I have decided to add a new feature to download and parse all available CMIP6 controlled variables and GCM outputs (ideas-lab-nus/epwshiftr#53). This part has almost been completed. I should be able to complete them at the end of this month. Then other issues related to documentation and the paper structure next month. Apologies for the slow moving. I hope I could solve all issues by the mid of next month. |
Thanks for the update @hongyuanjia! It's all good on the pacing, I just want to make sure you're not blocked by anything and have a plan for moving forward, which it sounds like you do! |
Hi @hongyuanjia, just checking in again to see how the updates are going? |
HI @hongyuanjia, just want to let you know that I'm going to be on vacation for the next three weeks. Hopefully you'll have a chance to wrap up the rest of your improvements by then! Thanks. |
/ooo July 15 until August 5 |
Hi @KristinaRiemer, I think most of the feature implementations have completed. I should be able to complete the documentation during this time period. Thanks! |
Hi @hongyuanjia, it looks like you completed a big chunk of work in your most recent commit in the software repo. Is the checklist of issues in a previous comment up to date? Have you been able to work on the documentation tasks? |
👋 folks. Just checking in here – are we still waiting for @hongyuanjia to make their updates? |
Thanks for checking in @arfon. @hongyuanjia, it looks like there are still some open issues based on the reviewers' feedback, how are those going? Is there anything blocking your progress? |
Hi @hongyuanjia, checking in to see if you have a timeline for completing these updates? |
Hi @hongyuanjia, I'm checking in again to see if you know when you will be able to address the updates? It appears that the last activity in the software repo was during October 2022. |
@hongyuanjia – it's more than a year since we heard back from you on this review. We will proceed to reject this submission as abandoned in 2 weeks from today if we don't hear back from you. |
I have sent an email to @hongyuanjia about this submission. |
@editorialbot reject This submission is being rejected due to inactivity on the part of the author. @KristinaRiemer @mitmat @bczernecki – thanks for all of your efforts here, sorry this one didn't work out! |
Paper rejected. |
Submitting author: @hongyuanjia (Hongyuan Jia)
Repository: https://github.com/ideas-lab-nus/epwshiftr
Branch with paper.md (empty if default branch):
Version: v0.1.3
Editor: @KristinaRiemer
Reviewers: @mitmat, @bczernecki
Archive: Pending
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:
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
@mitmat & @bczernecki, 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 @KristinaRiemer 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 @mitmat
✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @bczernecki
✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: