openjournals / joss-reviews

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

[REVIEW]: ERF: Energy Research and Forecasting #5202

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@asalmgren<!--end-author-handle-- (Ann Almgren) Repository: https://github.com/erf-model/ERF Branch with paper.md (empty if default branch): development Version: JOSS_paper Editor: !--editor-->@kthyng<!--end-editor-- Reviewers: @pratikvn, @rafmudaf Archive: 10.5281/zenodo.8102984

Status

status

Status badge code:

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

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

@pratikvn & @rafmudaf, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @kthyng 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

Checklists

📝 Checklist for @pratikvn

📝 Checklist for @rafmudaf

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.30 s (1010.8 files/s, 159177.8 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
C++                              92           4079           2393          20768
C/C++ Header                     56           1320            928           5869
reStructuredText                 31           1483            938           2847
SWIG                             27            439              1           1302
CMake                            31            148             88            754
Python                           10            169            319            694
make                             21            166            115            627
YAML                             11             83            142            533
Markdown                          2             75              0            416
Bourne Shell                     14             54             48            254
DOS Batch                         1             36              2            243
TeX                               1             10              0            109
JSON                              3              0              0             76
Bourne Again Shell                2              5             15              8
--------------------------------------------------------------------------------
SUM:                            302           8067           4989          34500
--------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1011

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.01370 is OK
- 10.1177/10943420211022811 is OK
- 10.1029/RG020i004p00851 is OK
- 10.2151/jmsj.87.895 is OK
- 10.1175/1520-0493(1963)091<0099:GCEWTP>2.3.CO;2 is OK
- 10.1007/BF00119502 is OK
- 10.1175/MWR3440.1 is OK
- 10.1175/1520-0469(2003)060<0607:CRMOTA>2.0.CO;2 is OK
- 10.1002/we.2017 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

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

pratikvn commented 1 year ago

Review checklist for @pratikvn

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

kthyng commented 1 year ago

I see @pratikvn is working on their review, and I've heard from @adam-m-jcbs by email about hoping to start their review this week.

adam-m-jcbs commented 1 year ago

Hi folks! I am very belatedly getting everything scaffolded for this and establishing a context / workflow so I can get working on and complete this review. I appreciate the patience, and @kthyng for reaching out.

adam-m-jcbs commented 1 year ago

[WIP] Review checklist for @adam-m-jcbs

Conflict of interest

Code of Conduct

General checks

Ann is the #1 committer to the repo, and I would assume a PI or co-I on one or many of this project's relevant grants. The other authors are relevant and appropriate as follows: 2 Aaron Lattanzi is CCSE, has commits in the repo 3 Riyaz Haque is LLNL, has commits in the repo 4 Pankaj Jha is LLNL, has commits in the repo 5 Branko Kosovic is NCAR, Director of Weather Systems & Assessment Program (this work falls within that scope), NCAR PI for relevant grants/projects 6 Jeffrey Mirocha is LLNL, appears to be one of this project's PIs? Also a core contributor to the "predecessor," WRF. 7 Bruce Perry is NREL, has commits in the repo, LES expertise 8 Eliot Quon is NREL, has commits in the repo, LES, multiscale expertise 9 Michael Sanders is SDSU, has commits in the repo, turbulence, numerical methods expertise 10 David Wiersema is LLNL, has commit in the repo, WRF contributor/principal 11 Donald Willcox is CCSE, has commits in the repo, CFD, AMReX, multiphysics expertise 12 Xingqiu Yuan is ANL, has second-most commits in the repo, project PI/co-I? 13 Weiqun Zhang is CCSE, has commits in the repo, CFD, multiphysics, multiscale, numerical methods, and AMReX expertise CCSE (LBL) is the core developer of the base framework this work builds on in addition to being a core ERF developer LLNL, NCAR, NREL, SDSU, and ANL do appear to collaborate across a range of grants and contributing a range of expertise / resources to realize this project, though the exact contributions and their relative weight in this project are not fully clear to me, though perhaps not needed for the purposes of this review.

To the best of my knowledge, the author list is complete and all contributors are duly credited. I see no clear evidence to the contrary nor ran into any reason during my review to suspect work has gone uncredited.

Suggestion to JOSS: it would speed review and be useful to readers if JOSS considered defining and implementing a practice of author contribution statements, adapting to fit JOSS' model and to not overly burden authors. It also aids reviewers in understanding different contributions. Especially in the context of JOSS and computational research, it can be very easy to fall into the trap of equating commits and contribution, erasing the many ways authors may be contributing outside of commits in a specific repo.

Functionality

Documentation

Software paper

adam-m-jcbs commented 1 year ago

@kthyng in the interests of integrity and transparency, I'd like to declare what could be perceived as a COI and also that I nonetheless feel able and sufficiently independent to carry out "an impartial scientific evaluation" for the purposes of this JOSS review.

The submitting author, Ann (@asalmgren), and Berkeley Lab's CCSE were collaborators on and essential to the funding of my PhD thesis work (especially the codes and frameworks I did my thesis work with and contributed to: AMReX, MAESTROeX). However, I earned my PhD in 2016 (ending active collaboration on funded projects) and am now based at NVIDIA. The authors of this submission and I are not collaborators currently (much to my dismay!) and I'm independent funding-wise, so I feel very able to carry out an impartial review. I just wanted to be careful and transparent with this disclosure.

I'll proceed with the review, Kristen, but please do let me know if this raises issues or if you'd like to discuss more.

kthyng commented 1 year ago

@adam-m-jcbs Given that none of the listed people were your direct supervisors, the collaborations were 6 years ago, and you feel able to carry out an impartial review, we are able to waive this conflict. Thank you for stating it clearing for the review issue.

kthyng commented 1 year ago

Just checking in. I see active conversations going on with @pratikvn!

@adam-m-jcbs When do you think you'll be able to get underway?

asalmgren commented 1 year ago

I believe as of tonight we have addressed all of the concerns shared by @pratikvn

If Adam is too busy perhaps we could ask another reviewer?

On Fri, Mar 31, 2023 at 8:50 AM Kristen Thyng @.***> wrote:

Just checking in. I see active conversations going on with @pratikvn https://github.com/pratikvn!

@adam-m-jcbs https://github.com/adam-m-jcbs When do you think you'll be able to get underway?

— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/5202#issuecomment-1492177895, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACRE6YR4ECBN7SNQKTXYWQLW634L5ANCNFSM6AAAAAAVMGJ3GI . You are receiving this because you were mentioned.Message ID: @.***>

-- Ann Almgren Senior Scientist; Dept. Head, Applied Mathematics Pronouns: she/her/hers

pratikvn commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

pratikvn commented 1 year ago

An update on the review from my side.

  1. I think the paper looks good to me.
  2. The software is lacking an API documentation. Atleast I cannot seem to find it on the webpage yet. There is feature and functionality documentation, but the API documentation is missing (I believe there is an open issue for this though: https://github.com/erf-model/ERF/issues/165).
  3. I had a few installation issues and I have reported them in the repository. I believe some of them have been resolved, but others need attention.
  4. I am not sure there are any particular performance claims made. They do mention performance portability and ability to run with the MPI + X model, but there have been no specific comparisons with respect to other libraries or performance in general, so I am not sure if I should be verifying something here.
  5. There are a few regression and functionality tests which are automated, but I cannot find any real-world examples.

So to summarize, three things are blocking IMO:

adam-m-jcbs commented 1 year ago

I'm getting ready for work and can't read comments on the mobile app at the moment. I believe from email subjects I saw fly by in my inbox I'm getting asked about this.

I'm still doing my best to review. Nothing has changed, as I said at the outset I have very bursty time available and may take a bit.

I'm working as fast as practiceable. Please be patient, or if y'all would like you can drop me as a reviewer, though that's not my preference.

adam-m-jcbs commented 1 year ago

Quick status update: I have set aside time this week to focus on this and plan to have completed a review by the end of this week, addressing each item on JOSS' checklist.

kthyng commented 1 year ago

@adam-m-jcbs Something must have come up! Will you be able to reschedule your review for this week?

@asalmgren Looks like you have some comments from @pratikvn to address in the meantime.

adam-m-jcbs commented 1 year ago

@kthyng yes, I was recently out sick last week and that has led to many things coming up.

I apologize for the slip. I can continue to work to complete this review and would like to, but have been disrupted and do not want to slow publication.

Being extremely conservative, I estimate I can complete this review at the latest by Friday May 19. I'll plan on sooner, but want to set realistic expectations.

I understand if that's too great a delay and an alternative reviewer is needed. I have prepared the environment for and started some of the functional review.

Just let me know how JOSS would like to move forward. I apologize to the editors and authors for this delay and disruption. For now, I'll plan to continue working on the review as able, the as able just became less than it was when I originally agreed to do the review.

kthyng commented 1 year ago

@adam-m-jcbs I'll see if I can recruit a new reviewer for now, but otherwise we'll fall back to you. I'll try two people and see if they're interested.

kthyng commented 1 year ago

@yhtang or @navidcy — would either of you be interested in reviewing this submission to JOSS? The review would take place in this github issue and issues created in the software repo itself. This review is underway but I'm recruiting a replacement reviewer, if possible.

navidcy commented 1 year ago

I'm sorry @kthyng but I'm really busy at the moment :(

kthyng commented 1 year ago

Sorry for my delay here — I had a stomach bug come through my household :(

@navidcy Thanks for your response and totally understand!

I think we better just stick with @adam-m-jcbs as reviewer and be thankful for his time. Let me know if anyone has concerns.

asalmgren commented 1 year ago

@kthyng -- I heard from another former JOSS reviewer who was interested in signing up again (to be a current JOSS reviewer) and willing to review this paper -- I believe he will be reaching out to you directly. Otherwise we'll be happy to stick with @adam-m-jcbs and appreciate him making the time.

kthyng commented 1 year ago

@asalmgren Ok I don't see anything so far but I'll keep an eye out.

rafmudaf commented 1 year ago

Hi @kthyng I believe I'm the other reviewer @asalmgren mentioned. I just updated my profile on joss.theoj.org. Is there anything else I should do before being qualified to review this submission?

kthyng commented 1 year ago

@rafmudaf do you have any conflicts with @asalmgren or the other authors? You can check here for more information: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html#joss-conflict-of-interest-policy

rafmudaf commented 1 year ago

I work within similar domains and sphere's of collaborators as the submitter, @asalmgren, but we have not directly worked together. I do, however, work with other contributors to ERF both within the same organization (NREL) and, at times, on shared projects. I should also note that I contributed to ERF very early in the project. The contributors graph shows that I made 33 commits in 2021, but the state of the software is considerably different now than it was then.

@kthyng I feel that I am able to remain impartial given that my work on ERF was a couple of years ago and the software has matured significantly in that time. As for my relationship with contributors, I feel entirely comfortable conducting a candid review and providing direct feedback. However, I also understand that one aspect of conflicts of interest is that their impact is not always evident to the people involved. I'll look out for your guidance on whether this COI can be waived, and please let me know if I can provide more information.

kthyng commented 1 year ago

Thanks for that information @rafmudaf. I think we can accept you as a reviewer with that background. We had a bit of an extended review timeline from @adam-m-jcbs (though now that timeline is not so far out...). @rafmudaf if you would like to jump in and could get started on your review soon, with the intention of being done before May 19 as @adam-m-jcbs was intending, then we could surely use your help to move this process along! I say the "intention" of being done since of course we don't know what issues will come up in the review process and dealing with everything properly is more important than finishing by a certain date, but getting started on the review soon and being able to potentially iterate on it is usually an important part of this process.

kthyng commented 1 year ago

@adam-m-jcbs Are you still up for doing this review in your original timeline or is it too late for that? We seem to have gotten jumbled together in this process.

kthyng commented 1 year ago

We seem to have gotten ourselves into a spot where it is unclear who our second reviewer is! @adam-m-jcbs Is your schedule opening up such that you can finish your review or do you not have time right now? Let us make a decision and move forward.

adam-m-jcbs commented 1 year ago

I'm sorry. Life and my day job have led to me losing all track of this and it's the day before the self-imposed deadline I set. There's no way I will make.

I apologize for wasting JOSS and the authors' time. I apologize for delaying publication. I cannot figure out what I need to do to get this review done quickly, even with all of this scaffolding and attempts at streamlining JOSS has done, I ended up having to spend most of my time on the author relevance task and barely even got to cloning the code after hours of effort.

I cannot get environments setup to do the review, cannot find the weeks of time needed to focus on this. I thought the streamlining, as well as the computational and automated nature of this work, would make that efficient enough to do, but that's not the case, for me at least. This review is too much effort for me, and I apologize for misunderstanding that.

@kthyng , it seems I need to withdraw as a reviewer, and it seems prudent to remove me from the list of potential JOSS reviewers. This process isn't practicable for me, it seems. I don't want to waste other authors' and editors' time again.

kthyng commented 1 year ago

@adam-m-jcbs It does look like you dove in deep working to understand author information, beyond what we expected. I'm sorry you've had a poor experience reviewing here — if you end up wanting to, I'd invite you to follow up to share your experience to see if we can improve things for reviewers, or at least to better communicate the tasks we're requesting. I'll also share your comment with the lead editorial team.

I'll remove you as a reviewer and from our list of reviewers, as requested.

kthyng commented 1 year ago

@editorialbot remove @adam-m-jcbs as reviewer

editorialbot commented 1 year ago

@adam-m-jcbs removed from the reviewers list!

kthyng commented 1 year ago

@rafmudaf do you want to take over as reviewer?

rafmudaf commented 1 year ago

@kthyng I would like to do this review, but the timing is not great - I'm at a conference all next week and then I'll be out of the office for two weeks after that. @asalmgren is the JOSS review tied to a particular time frame (i.e. DOE milestone)? If not, I'm happy to commit to completing this as soon as I'm back the week of June 19.

asalmgren commented 1 year ago

@rafmudaf -- we had originally been hoping for an earlier time frame but I think at this point our best path forward is for you to do the review when you free up in June. @kthyng -- is this ok with you?

kthyng commented 1 year ago

Yep good for me. I'll add @rafmudaf as reviewer and we'll pick this up in a few weeks. Thanks all!

kthyng commented 1 year ago

@editorialbot add @rafmudaf as reviewer

editorialbot commented 1 year ago

@rafmudaf added to the reviewers list!

rafmudaf commented 1 year ago

Thanks @kthyng @asalmgren. It's on my TODO list for when I'm back. Talk to you soon!

rafmudaf commented 1 year ago

Review checklist for @rafmudaf

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

rafmudaf commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

kthyng commented 1 year ago

@pratikvn Could you summarize if you still have any outstanding questions/comments for this JOSS submission?

@rafmudaf Great to see you back!

pratikvn commented 1 year ago

@kthyng, From my side the only thing that is missing are some examples:

Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).

kthyng commented 1 year ago

@asalmgren Please see the reviewer comment above to address. ☝️

asalmgren commented 1 year ago

@pratikvn -- We have restructured the Exec directory to make it clear which types of tests are which, and added README files to each Exec/* subdirectory. In addition, we have added more information under Getting Started / Building to describe how to build and run tests. We have also removed duplicative test from the README.md file and instead we point to Getting Started. We believe this should show the interested user how to build and run the code.