search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[REVIEW]: Enhanced software and platform for the Town Energy Balance (TEB) model #2008

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @dmey (D. Meyer) Repository: https://github.com/teb-model/teb Version: 4.0.0 Editor: @mjsottile Reviewers: @ethan-nelson, @jayten Archive: 10.5281/zenodo.3887081

Status

status

Status badge code:

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

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

@jayten & @ethan-nelson, 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 @mjsottile know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @jayten

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @ethan-nelson

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @rasmussn, @ethan-nelson 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:

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

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

Can't find any papers to compile :-(

dmey commented 4 years ago

@whedon generate pdf from branch joss-paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1023/A:1002463829265 is OK
- 10.5194/gmd-5-1377-2012 is OK
- 10.5194/gmd-11-1929-2018 is OK
- 10.1002/joc.3415 is OK
- 10.5194/gmd-5-433-2012 is OK
- 10.5194/gmd-10-2801-2017 is OK
- 10.1175/WAF-D-11-00064.1 is OK
- 10.3389/fenvs.2014.00014 is OK
- 10.21105/joss.01137 is OK
- 10.5065/1dfh-6p97 is OK
- 10.1016/j.aeaoa.2019.100042 is OK
- 10.1007/s00703-008-0320-9 is OK
- 10.21105/joss.01468 is OK
- 10.1007/BF01025401 is OK
- 10.1007/s007030070003 is OK
- 10.1175/2009MWR2750.1 is OK
- 10.1007/s10546-009-9414-2 is OK
- 10.1175/1520-0493(1998)126<1373:TOCMGE>2.0.CO;2 is OK
- 10.1023/A:1016509614936 is OK
- 10.1007/s10546-006-9091-3 is OK
- 10.1175/2009JAMC2153.1 is OK
- 10.5281/zenodo.3562310 is OK

MISSING DOIs

- https://doi.org/10.1175/1520-0450(2004)043<0312:mtsebo>2.0.co;2 may be missing for title: Modeling the Surface Energy Balance of the Core of an Old Mediterranean City: Marseille
- https://doi.org/10.1007/s00585-997-0090-6 may be missing for title: The Meso-NH Atmospheric Simulation System. Part I: Adiabatic Formulation and Control Simulations
- https://doi.org/10.1175/1520-0450(2002)041<1011:eotteb>2.0.co;2 may be missing for title: Evaluation of the Town Energy Balance (TEB) Scheme with Direct Measurements from Dry Districts in Two Cities

INVALID DOIs

- None
dmey commented 4 years ago

@whedon generate pdf from branch joss-paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

dmey commented 4 years ago

@mjsottile, @rasmussn, @ethan-nelson I have added the three missing DOIs. Will keep the summary paper on a separate branch (joss-paper) in case of any changes and will only merge it into master once you are all happy with it.

dmey commented 4 years ago

Hi @mjsottile, @rasmussn, @ethan-nelson I was wondering if you had a chance to look at this? Thanks

mjsottile commented 4 years ago

:wave: @rasmussn, @ethan-nelson Please let me know if you are having any trouble with the items on the checklist at the top of this thread for the review.

ethan-nelson commented 4 years ago

@mjsottile @dmey working through it--should have things completed by tomorrow US time!

ethan-nelson commented 4 years ago

This is my first review with JOSS, so thanks for this opportunity. Code looks to be in good shape. Tests run fine and physical values in the raw output of testing are reasonable. Modules are documented on the corresponding website, model scientific documentation is available, and namelist parameters are defined. The paper covers the important points of the software and highlights where it aligns with existing packages.

Responding to a few “unchecked” points in the checklist that are minor:

General checks

Functionality

/home/enelson/review/teb/external/minimal-dx/src/shared/minimal_dx_fan.f90:99.64:

      error stop 'Fan Mode must either be 1 for on or 0 for off'
                                                                1
Error: ERROR STOP statement not allowed in PURE procedure at (1)
gmake[2]: *** [CMakeFiles/teb.dir/external/minimal-dx/src/shared/minimal_dx_fan.f90.o] Error 1
gmake[1]: *** [CMakeFiles/teb.dir/all] Error 2
gmake: *** [all] Error 2

After removing the error message, the model compiled just fine. This is a fairly old gcc version though so it may not warrant a change.

Documentation

Other feedback:

dmey commented 4 years ago

@ethan-nelson many thanks for your useful comments. I will get back to you when I receive the second review and can more easily combine changes in the code in case of further feedback.

mjsottile commented 4 years ago

To update, I've reached out to the other reviewer via email to see what their status is. Hopefully will have an update relatively soon.

mjsottile commented 4 years ago

I still have not heard back, so I am going to try to find an additional reviewer to un-stall the review. Sorry for the delays @dmey.

dmey commented 4 years ago

I still have not heard back, so I am going to try to find an additional reviewer to un-stall the review. Sorry for the delays @dmey.

Thank you @mjsottile and no problems!

mjsottile commented 4 years ago

:wave: @dvalters @AnsleyManke @andreas-h Would any of you be available/interested in providing a review for this JOSS submission? I am currently looking for a reviewer to replace someone who has let me know that they are unable to complete the review.

mjsottile commented 4 years ago

:wave: @rchg @irnaka @jayten Hello - we are looking for an additional reviewer for this JOSS submission to replace a reviewer who can no longer do a review. I found your names on the potential reviewers spreadsheet based on your preferred languages and domains of expertise. Would any of you be available for a review?

andreas-h commented 4 years ago

Sorry, cannot do it.

AnsleyManke commented 4 years ago

This is not in my area of expertise, sorry.

irnaka commented 4 years ago

Sorry, this is not my area of expertise.

============================== Theodosius Marwan IRNAKA

marwan.irnaka@gmail.com marwan.irnaka@gmail.com +62852 2803 5489 <%2B6285228035489> +336 47 08 81 33

On Wed, 19 Feb 2020 at 22:19, Matthew Sottile notifications@github.com wrote:

👋 @RCHG https://github.com/RCHG @irnaka https://github.com/irnaka @jayten https://github.com/jayten Hello - we are looking for an additional reviewer for this JOSS submission to replace a reviewer who can no longer do a review. I found your names on the potential reviewers spreadsheet based on your preferred languages and domains of expertise. Would any of you be available for a review?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2008?email_source=notifications&email_token=ABGZYFZKXMBSP7FN5ZHGPLDRDWO4NA5CNFSM4KE7IJGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMJW2TA#issuecomment-588475724, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZYF2AMVSXFFEHKXXNDXDRDWO4NANCNFSM4KE7IJGA .

jayten commented 4 years ago

@mjsottile If you are still looking for a reviewer, I will be happy to help with this JOSS submission.

mjsottile commented 4 years ago

@whedon remove @rasmussn as reviewer

whedon commented 4 years ago

OK, @rasmussn is no longer a reviewer

mjsottile commented 4 years ago

@whedon add @jayten as reviewer

whedon commented 4 years ago

OK, @jayten is now a reviewer

mjsottile commented 4 years ago

Thank you @jayten for agreeing to review. I’ve added you as a reviewer and updated the checklist at the top of the issue to include you. Please let me know if you have any questions or need any assistance in completing your review.

jayten commented 4 years ago

I have gone through the review process for the teb repository. I was able to install (on Ubuntu18 and MacOs 10.13) and run test cases with instructions provided in the documentation. The software solves the intended problem well. The software is well written and documented. However, the flavor of the FORTRAN code style is old, owing to the use of old legacy code. For example, "impnone.fh" just contain an 'implicit none' statement. Although code style is not a major issue, it can be improved over time with the contribution of others. I would suggest @dmey to add, at least a simple, CONTRIBUTING.md for interested individuals to contribute to teb.

License

I have issues with the license file: CeCILL-C-V1. The license file mentioned is not a part of the OSI. Additionally, some old source files (like 'modei_flxsurf3db.f') mention different license in the header, which is also not part of the OSI. I want @mjsottile to take a decision on this issue.

Paper

@dmey , make sure all the contributors are at least acknowledged in the paper.md. For example, P. Le Moigne, mentioned as the author of the abort1_sfx.f90 source file, is neither mentioned in the author list nor the acknowledgment.

References

Missing DOI for bib entry: rozoff_2003_J.Appl.Meteorol

mjsottile commented 4 years ago

@jayten Thanks for the great and prompt review. I will look into the issues you raise when I return from travel next week. I need to look more into the CeCILL licenses - as I understand them from looking at them a number of years ago, they are generally recognized as similar to the FSF and OSI licenses, but originated in Europe independently of the more common licenses that largely originated in the US. I’ll do some reading and possibly ask the other JOSS editors for their opinions on the issue.

In the meantime @dmey, if you would like to look at the issues related to contributors and reference DOIs, that would be useful.

danielskatz commented 4 years ago

JOSS requires submitted software to have an OSI-approved license. It looks like we (actually I) missed this originally, for which I apologize to the author and reviewers.

danielskatz commented 4 years ago

👋 @dmey - Is it possible to change (or dual license) to/with an OSI-approved license? If not, we will have to ask you to withdraw this submission or we will have to reject it.

danielskatz commented 4 years ago

Note that https://opensource.org/licenses/CECILL-2.1 is an OSI-approved license - but CeCILL-C-V1 does not appear to be

danielskatz commented 4 years ago

Finally, I see on https://en.wikipedia.org/wiki/CeCILL:

Version 2.1 was released in June 2013. It allows relicensing to the GNU Affero General Public License and the European Union Public License as well as the GPL, and clarifies the language that requires licensees to give access to the source code (which had previously caused rejection of version 2.0 by the Open Source Initiative[3]).

Note that CeCILL v1 already allowed replacing a CeCILL v1 license by CeCILL v2, so all software previously licensed with CeCILL v1 in 2004 can be licensed with CeCILL v2, with legal terms enforceable as authentic not only in French but in English too.

dmey commented 4 years ago

Hello all, many thanks for your prompt comments and feedback -- I will get back in the next 10 days or so.

dmey commented 4 years ago

@mjsottile and @danielskatz I have discussed these points with Masson given that he is the co-author who developed the physical model in 2000 and wrote most of the original code. We should be able to make most of the changes as requested but would to clarify a couple of points with you first before going ahead with the changes:

  1. Licence: OK -- this can be upgraded to CECILL-2.1 but will ask Masson to approve this as it will affect the whole software and not only my changes/enhancements.
  2. Acknowledgments: in the repository there are a couple of files authored by Le Moigne. I have double checked this again with Masson but it looks like these files were included from a 'sister' model (SURFEX) and Le Moigne was not a TEB contributor. Do you still require this to be acknowledged in the summary paper?
  3. Zenodo: We had not yet deposited the software on Zenodo however, given that my contribution is recent and specifically about the enhancements as described in the paper, I think that the software version reference should reflect all the software contributors including those acknowledged in the summary paper and with a different order – i.e. Masson et al. (2020).
danielskatz commented 4 years ago

The following are my thoughts as the AEiC on duty when this came up. @mjsottile and the currently on-duty @openjournals/jose-eics might also have thoughts.

  1. Licence: OK -- this can be upgraded to CECILL-2.1 but will ask Masson to approve this as it will affect the whole software and not only my changes/enhancements.

A license change needs to be approved by all contributors in theory, since they are the copyright holders.

  1. Acknowledgments: in the repository there are a couple of files authored by Le Moigne. I have double checked this again with Masson but it looks like these files were included from a 'sister' model (SURFEX) and Le Moigne was not a TEB contributor. Do you still require this to be acknowledged in the summary paper?

If this software was not developed for the project but is included (in some sense, as a dependency), it likely makes sense to acknowledge that dependency. The author of this dependent software would not need to be an author of this software, in my opinion.

  1. Zenodo: We had not yet deposited the software on Zenodo however, given that my contribution is recent and specifically about the enhancements as described in the paper, I think that the software version reference should reflect all the software contributors including those acknowledged in the summary paper and with a different order – i.e. Masson et al. (2020).

I'm not sure I understand this. Both the JOSS paper and associated Zenodo (or other) archival repository deposit should contain the same list of authors, in the other determined by the authors, and with the list being determined by the authors. The authors should include all significant software contributors, plus potentially others who contributed to the project in ways that may not be captured in the repository (e.g. software design, user support, ...)

dmey commented 4 years ago

I'm not sure I understand this. Both the JOSS paper and associated Zenodo (or other) archival repository deposit should contain the same list of authors, in the other determined by the authors, and with the list being determined by the authors. The authors should include all significant software contributors, plus potentially others who contributed to the project in ways that may not be captured in the repository (e.g. software design, user support, ...)

I appreciate that this may be a bit confusing -- the reason I have asked is because we would rather not fork the original model but instead, improve and move the model to GitHub (i.e. this paper) and continue future development from this point forward. Therefore, whereas in the paper the authorship is clear for the work that was done in improving TEB, I think that for the associated Zenodo archive, it would be proper to keep the software name and authorship to reflect the contributions made over the years of development (i.e. leave name as TEB -- as also reflected in GitHub repository-- thus avoiding the creation of a separate fork for this specific submission). This is something that has been discussed with the TEB developers and seemed to make most sense.

danielskatz commented 4 years ago

👋 @arfon - any thoughts on the above comment?

arfon commented 4 years ago

If I understand the situation, the paper will have a shorter author list than the Zenodo archive? I don't think this is ideal but I think it's probably OK to allow this given the justification.

mjsottile commented 4 years ago

I agree. It seems that the only outstanding item is to make sure all necessary parties consent to the update from CeCILL1 to CeCILL-2.1 such that the license will conform to the JOSS requirements. Once you've indicated that is complete, I can scan through the state of the submission to push it to the next stage if it looks ready.

mjsottile commented 4 years ago

@whedon generate pdf from branch joss-paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

mjsottile commented 4 years ago

@whedon check references from branch joss-paper

whedon commented 4 years ago 
Attempting to check references... from custom branch joss-paper
whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1023/A:1002463829265 is OK
- 10.1175/1520-0450(2004)043<0312:MTSEBO>2.0.CO;2 is OK
- 10.5194/gmd-5-1377-2012 is OK
- 10.5194/gmd-11-1929-2018 is OK
- 10.1002/joc.3415 is OK
- 10.5194/gmd-5-433-2012 is OK
- 10.5194/gmd-10-2801-2017 is OK
- 10.1175/WAF-D-11-00064.1 is OK
- 10.3389/fenvs.2014.00014 is OK
- 10.21105/joss.01137 is OK
- 10.5065/1dfh-6p97 is OK
- 10.1016/j.aeaoa.2019.100042 is OK
- 10.1007/s00703-008-0320-9 is OK
- 10.21105/joss.01468 is OK
- 10.1007/BF01025401 is OK
- 10.1007/s007030070003 is OK
- 10.1175/2009MWR2750.1 is OK
- 10.1007/s10546-009-9414-2 is OK
- 10.1175/1520-0493(1998)126<1373:TOCMGE>2.0.CO;2 is OK
- 10.1023/A:1016509614936 is OK
- 10.1007/s00585-997-0090-6 is OK
- 10.1007/s10546-006-9091-3 is OK
- 10.1175/2009JAMC2153.1 is OK
- 10.5281/zenodo.3562310 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1175/1520-0450(2002)041<1011%3AEOTTEB>2.0.CO%3B2 is INVALID
mjsottile commented 4 years ago

@dmey In addition to the license issue, both reviewers have not checked off the box related to community guidelines. It looks like the paper provides guidance for users wishing to contribute, but the README or other documents in the repository do not. You should add some text to the README or a separate CONTRIBUTING.md file for users who encounter the package but do not read the JOSS paper.

Also, it looks like there is an invalid DOI in the paper - can you repair that? (See the "check references" result above).

Once you've had a chance to address these issues, please take a look at the reviewer comments again to make sure you've addressed both of their concerns. At that point I'll check in with them again to see if they're satisfied with the outstanding boxes from their checklists.

dmey commented 4 years ago

Thank you for the clarifications -- I will look at the issues in the next couple of weeks and get back.