JOSS reviewer 1 feedback.

[pratikvn]

Hello,

I am reviewer for your JOSS paper on ERF. The repository and its aims are very interesting and I look forward to the review.

To make the review easier for me and for you, I am creating this main issue to keep track of my review regarding general suggestions and issues. To keep clutter to a minimum, I will try to keep all my questions in here as well.

If I face any compilation/running issues, I will create new issues for those and link them back here so that you can also keep track of the progress.

Please let me know if you would prefer another way instead.

Repository related suggestions:

• I see that you already have a license.txt file, which is great but due to the way github works, I think it would be useful to move that to a file named LICENSE or LICENSE.md, which allows the github webpage to more promimently display your license. See their documentation for more information

Paper suggestions

• The paper itself is well-written. One suggestion I would have is to split up the large Summary section into a smaller section with a high level overview and a Features section that explains the different features, models and options that the software provides.

Some general questions:

• Could you please explain the rationale for the author list ? I see that some authors who are in the github contributors are not part of the paper.

Documentation suggestions

• I see that ERF depends on AMREX, RRTMGP and on YAKL. While AMREX has been mentioned as a dependency, the other two have not. I think this should be clarified in the installation section in the documentation.
• The Cdash link for nightly test seems to not exist now ? https://my.cdash.org/index.php?project=ERF
• I really like the Build/ directory with sample cmake commands. But I did not find this mentioned in the documentation. I think this would be useful to have in the docs.
• API documentation
• Some basic examples and how to run them

Compilation issues

• Resolve JOSS review: Compilation issues. #1015

Run and test issues

• Resolve JOSS review: Failing tests #1014

[asalmgren]

Thank you for your questions/comments/suggestions! Here I address two of the comments and we will respond to the others directly once we have addressed them

1. We have moved license.txt to LICENSE.md
2. Re the author list -- the github contributor history is longer than it should be due to how this code was initially created on github -- it was started by copying, then gutting, an existing code, which had itself started that way. The github contributors who are not on the author list did not contribute to the ERF code itself.

[pratikvn]

@asalmgren , Thank you for the clarifications.

[dwillcox]

@pratikvn I've addressed the point you raised about the paper in #1022 -- the paper is now organized into a high level overview and a set of feature sub-sections. Can you confirm this addresses your concerns?

[AMLattanzi]

The deprecated cdash link was removed from the documentation and use of the Build/ directory was expanded upon in PR 1013. Additionally, documentation was added on RRTMGP and on YAKL in PR 1016.

[pratikvn]

Thank you @dwillcox. I think it looks good to me now. Thank you @AMLattanzi for the documentation updates. I would consider all these resolved now.

[pratikvn]

I see that some basic doxygen generated API documentation has been added. I think it would also be helpful for users to have some class and function documentation in the source files which is then also in the API documentation, but I understand that requires more substantial work, so I think it is okay to not have it as a requirement for the paper.

I think it would be useful to have atleast a couple of examples. You could even just copy and repurpose your most representative regression tests, and combine that with the theory you have on your webpage, showing how to run it and what is the expected output.

[asalmgren]

@pratikvn -- all of your comments/ feedback are very helpful. We are still in process of adding the annotations for doxygen, and will let you know when we have more complete coverage. We are also planning to follow your suggestion to post several examples -- we are having internal discussions on which examples to post. Many thanks.

[asalmgren]

@pratikvn -- I believe we have completed the doxygen documentation (it's not perfect but relatively comprehensive) -- I will consider that part of the issue closed unless you tell us otherwise. We are still working on the examples.

[pratikvn]

@asalmgren , Thank you. I see that it is mentioned on your main webpage (https://erf.readthedocs.io/en/latest/index.html) as a line at the end, but that is easy to miss. Maybe you can also add it to your README on github ? But otherwise, it looks good to me.

[asalmgren]

@pratikvn Done :-)

[pratikvn]

As I mentioned in the other issue, adding a section that the tests can be used as examples would be helpful for the users. Otherwise, all points were addressed and I will close this issue now.