search

openjournals / joss-reviews

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

[REVIEW]: AIBECS.jl: The ideal tool for exploring global marine biogeochemical cycles. #3814

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: @briochemc (Benoit Pasquier) Repository: https://github.com/JuliaOcean/AIBECS.jl Version: v0.11.2 Editor: @kthyng Reviewers: @dankelley, @zhenwu0728 Archive: 10.5281/zenodo.5787531

: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

Status badge code:

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

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

@dankelley, @zhenwu0728 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 @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 ✨

Review checklist for @dankelley

✨ 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 @zhenwu0728

✨ 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

whedon commented 3 years ago

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

  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 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.05 s (1386.9 files/s, 139568.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           45           1080            856           3503
Markdown                         7            105              0            291
YAML                             9             12              0            288
TeX                              1             35              0            254
TOML                             2              5              0             77
JSON                             1              0              0             35
-------------------------------------------------------------------------------
SUM:                            65           1237            856           4448
-------------------------------------------------------------------------------

Statistical information for the repository 'ceb9abfad540c058d4e7a0b6' was
gathered on 2021/10/11.
No commited files with the specified extensions were found.
whedon commented 3 years ago

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

 Can't find any papers to compile :-(
kthyng commented 3 years ago

@whedon generate pdf from branch JOSSpaper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch JOSSpaper. 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:

dankelley commented 3 years ago

@kthyng I filled in the boxes, and wrote up some notes as I was reading. I am putting those notes below. I don't quite see how to submit a detailed review otherwise, so I hope this is useful. If you're looking for a Boolean answer, mine is "publish".

I will be able to look at this again tomorrow if desired but after that I won't have much time. I enjoyed reading this and trying out the software, and I thank you for considering me. Dan.

Recommendation

I recommend accepting this submission. It is a welcome addition to the field, and it would be to the credit of JOSS to publicize it to a wide range of educators/researchers.

I have made some comments below, in the major/minor format that I normally use for scientific journals. I am not sure whether this is useful for a JOSS manuscript, but I didn't see any other way to provide more detailed comments, after I finished the checklist. As can be seen, I have a few suggestions that might improve the clarity of the writing, but I leave it up to the authors to decide whether to follow them (except for the minor comment about a broken citation).

Were this a conventional scientific paper, I would have appreciated a bit more about the class of modelling frameworks supported by this project, and about any related limitations. However, for JOSS, I think what the authors have said on this matter is wholly sufficient.

As for JOSS, I do think it would be helpful to add a few sentences about why Julia is chosen for the project, because their addition might give the work a broader impact. (Basically, Julia opens up doors that are closed to other systems, and folks who have not used it might not be aware just how important this is.) However, I leave the choice up to the authors.

I am always happy to see software aimed at more than just a single project. It's even better when developers take the time to educate others about how their creations might help others in the work. I think JOSS is designed specifically for this sort of thing, and I think the authors deserved to be thanked not just for their authorship of the software, but also for considering JOSS as a way to expose the work to a wider audience.

Major Comments

The manuscript outlines in reasonable detail the need for the package, although I would have appreciated more commentary on the focus on steady-state models, which is a limitation that is certainly acceptable in many applications, although it is somewhat swept under the rug here. On the technical side, the paper might have been strengthened by discussing in more detail why Julia is well-suited to this application.

As for the github site and the code, both are of high quality. I worked through the 'age' tutorial, and it is straightforward and informative -- a model of how to do this sort of thing, in my opinion.

Minor comments

kthyng commented 3 years ago

@dankelley Thank you so much for your quick review! It is fine to put comments in the review issue like this. As there become more, we ask that they go to the software repository under review itself as issues that link back to this review issue (no need to change anything in this case if there is nothing to add).

I just want to verify: most of your comments are about the paper, while most of the JOSS review itself tends to be focused on the software. I see you said that you worked through a tutorial so it sounds like it went well. Can I just verify you were able to install the software and use it then? Thanks.

dankelley commented 3 years ago

Oh, yes, installation was trivial (as is usually the case with Julia) and the sample code that I tried all worked. I went through one of the tutorials in detail, and it was not just correct, but also educational -- the authors have gone beyond merely documenting the software as such, and have also explained the gist of the relevant calculations. This is a useful, working, and splendidly documented package.

dankelley commented 3 years ago

Sorry, that last comment was in reply to @kthyng.

kthyng commented 3 years ago

Sounds great, thanks for your fast responses and review @dankelley!

kthyng commented 3 years ago

@whedon add @zhenwu0728 as reviewer

whedon commented 3 years ago

OK, @zhenwu0728 is now a reviewer

kthyng commented 3 years ago

@zhenwu0728 I think you should be good to go now. Let me know if you have any problems, and thanks!

zhenwu0728 commented 3 years ago

Hi @kthyng ! I've gone through the paper and documentation and tried two of the examples ("ideal age" and "PO4-POP"). This package is well developed and organized. I really like the examples and How-to guides. I opened an issue in AIBECS.jl with a few minor comments and suggestions.

kthyng commented 3 years ago

Excellent! Let's hear from @briochemc to see responses to your comments.

kthyng commented 3 years ago

@briochemc Looks like we are waiting to hear back from you on your reviewer input.

briochemc commented 3 years ago

@kthyng Yes! I've been working (slowly) on the code revisions according to @zhenwu0728's code comments! (I already merged a PR with one fix for output display, and I am trying options for moving the notebooks from Jupyter to Pluto)

briochemc commented 3 years ago

@whedon commands

whedon commented 3 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository
kthyng commented 3 years ago

@briochemc Ok perfect! Carry on.

briochemc commented 2 years ago

@whedon generate pdf from branch JOSSpaper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch JOSSpaper. Reticulating splines etc...
whedon commented 2 years ago

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

briochemc commented 2 years ago

@whedon generate pdf from branch JOSSpaper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch JOSSpaper. Reticulating splines etc...
whedon commented 2 years ago

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

briochemc commented 2 years ago

@whedon generate pdf from branch JOSSpaper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch JOSSpaper. Reticulating splines etc...
whedon commented 2 years ago

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

briochemc commented 2 years ago

@whedon generate pdf from branch JOSSpaper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch JOSSpaper. Reticulating splines etc...
whedon commented 2 years ago

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

briochemc commented 2 years ago

@whedon generate pdf from branch JOSSpaper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch JOSSpaper. Reticulating splines etc...
whedon commented 2 years ago

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

briochemc commented 2 years ago

Dear @kthyng,

We have revised the manuscript according to @dankelley's and @zhenwu0728 's review comments in PR #87 (for convenience: link to the diff of the manuscript since its first submission).

We have fixed all the code issues mentioned in Issue #83 by @zhenwu0728 in PR #85 and PR #87 except for the suggestion to port the notebooks from Jupyter to Pluto. This is because of breaking changes in Pluto itself that made it incompatible with the current parameter-handling implementation of AIBECS, which relies on function-overloading macros. While we hope to eventually revisit the parameter internals and interface to rely on fewer macros and make it Pluto compatible, we believe such extensive code revisions are out of the scope of @zhenwu0728's suggestion. We, therefore, hope that our revisions in PR #85 are sufficient. (It makes sense for Issue #83 to remain open, however, as a reminder to make the transition from Jupyter to Pluto in the future).

Overall, we believe @dankelley's and @zhenwu0728 's comments have helped us improve the software and the manuscript. (They are now acknowledged in the manuscript.)

Below you will find our point-by-point replies to the reviewers' comments.

We hope you will find that our revisions are sufficient for publication in JOSS.

On behalf of the authors, Best Regards, Benoit Pasquier

Response to reviewer 1 (@dankelley) comments

I recommend accepting this submission. It is a welcome addition to the field, and it would be to the credit of JOSS to publicize it to a wide range of educators/researchers.

We thank @dankelley for his recommendation.

I have made some comments below, in the major/minor format that I normally use for scientific journals. I am not sure whether this is useful for a JOSS manuscript, but I didn't see any other way to provide more detailed comments, after I finished the checklist. As can be seen, I have a few suggestions that might improve the clarity of the writing, but I leave it up to the authors to decide whether to follow them (except for the minor comment about a broken citation).

The broken citation has been fixed and the suggestions have been broadly followed (details below).

Were this a conventional scientific paper, I would have appreciated a bit more about the class of modelling frameworks supported by this project, and about any related limitations. However, for JOSS, I think what the authors have said on this matter is wholly sufficient.

We have added "steady-state" in multiple sentences to clarify (repeatedly) that this is the class of modelling currently in the scope of AIBECS.

As for JOSS, I do think it would be helpful to add a few sentences about why Julia is chosen for the project, because their addition might give the work a broader impact. (Basically, Julia opens up doors that are closed to other systems, and folks who have not used it might not be aware just how important this is.) However, I leave the choice up to the authors.

We have added a sentence clarifying that AIBECS owes its user-friendliness and efficiency to the Julia language.

I am always happy to see software aimed at more than just a single project. It's even better when developers take the time to educate others about how their creations might help others in the work. I think JOSS is designed specifically for this sort of thing, and I think the authors deserved to be thanked not just for their authorship of the software, but also for considering JOSS as a way to expose the work to a wider audience.

We thank @dankelley for this comment.

Major Comments

The manuscript outlines in reasonable detail the need for the package, although I would have appreciated more commentary on the focus on steady-state models, which is a limitation that is certainly acceptable in many applications, although it is somewhat swept under the rug here. On the technical side, the paper might have been strengthened by discussing in more detail why Julia is well-suited to this application.

We have added "steady-state" in multiple sentences to clarify (repeatedly) that this is the current scope of AIBECS. We have also added a sentence explaining why Julia was chosen.

As for the github site and the code, both are of high quality. I worked through the 'age' tutorial, and it is straightforward and informative -- a model of how to do this sort of thing, in my opinion.

Minor comments

  • Overall, the citations are overly heavy, in my opinion. I prefer writing that cites only papers that a reader might reasonably want to consult, to learn more about a particular point raised in the text. As an example, L103 consists entirely of citations, and all readers are told about them is that they relate to parameters relating to the carbon cycle. This seems to be insufficient guidance, at least for readers like me. I am not objecting to this style, because I know it to be common in some fields.

We appreciate this comment, which is similar to a comment by @zhenwu0728. The purpose of this list of references was not to point the reader to more detailed studies of particular points. Instead, it was intended to provide links to previous relevant modelling work that could have benefited from a framework like the one provided by AIBECS. However, we agree that trimming was appropriate here, and we have thus removed most of that paragraph although we have kept references to a few high-profile publications. In our opinion, the manuscript now gets straighter to the point.

  • I suggest removing "ideal" from the title. Let readers decide that.

The title now reads "A tool (...)" instead of "The ideal tool (...)".

  • Para 1 (L2:18) I think it might be useful to add a few sentences on why Julia is a good choice for this project. Otherwise, the paper might only be read by folks who are already familiar with the language.

We have added "Written in Julia — chosen for of its combined expressive power and efficiency" a bit later in the text, after introducing the (MATLAB) AWESOME OCIM.

  • L27: This suggestion by Box about all models being wrong was essentially a discussion point, not a scientific finding or a mathematical conclusion. It might help to rewrite that sentence in a form like "Box (1979) has remarked that ", to make things a bit clearer. In some existential sense, perhaps everything is wrong.

We have revised the text as per the suggestion.

  • L28: Readers might take the authors to mean that parameters only arise when assumptions are made. This is not entirely true. Another use of parameters is to use a model to make predictions for a range of cases. While I do not object to the phrasing as it is, I might suggest L28 s/give rise/can give rise/. (Here I am using a sort of sed instruction to indicate a suggested edit, although the 28 refers to a line number as printed, not in the file. I'll use that notation throughout this review.)

We have revised the text accordingly.

  • L31: It's not clear that minimizing model-observation misfit always improves the skill of a model, if the latter is (I think, reasonably) taken to indicate prediction of a yet-unobserved state. Overfitting can yield poor predictions, sometimes. Perhaps L32 s/which also improves/which may also improve/ might help a bit, if the authors agree that there is a slight chance of confusion here.

We have revised the text to say "may improve".

  • L73: the Taburet_etal_2019 citation is broken.

This citation has been fixed.

Response to reviewer 2 (@zhenwu0728) comments

I've worked through "ideal age" and "PO4-POP" examples, and they are well documented and informative. I only have a few minor comments for the package and the paper.

  • The examples are great, but Binder is not working properly (tried a few times to launch Binder and it crashed while running the model, possibly because of the limit of memory). I suggest the authors may use Pluto.jl and PlutoSliderServer.jl to host examples.

It is correct that Binder does not work "well" for these tutorials. This is because one has to load AIBECS (and its dependencies) and download circulations and data on a remote machine.

Translating them to Pluto is a great idea but, unfortunately, the latest version of Pluto (v0.17.2) is not compatible with AIBECS.jl. The current parameter interface of AIBECS.jl (which works with earlier Pluto versions, such as v0.15.0) relies on FieldMetadata.jl macros that extend functions that are imported from FieldMetadata.jl (e.g., units, bounds). However, in Pluto v0.17.2, imported functions now cannot be extended in separate cells (see issue 846).

In response, we have added a note at the start of each tutorial and each how-to guide mentioning that binder might be slow.

Eventually, we might entirely rewrite the parameter internals and the user interface (without macros), but we believe that such a revision is out of the scope of @zhenwu0728's suggestion.

  • A better show() is needed for OCIM2.load(), F_and_∇ₓF, etc. Now it's hard to get any useful information from the output.

We have fixed the output issues of OCIM2.load() by not returning the often-unused He-flux fields. These can still be loaded if the keyword argument FeFluxes=true is provided. We have also fixed the output of F_and_∇ₓF by revising the interface slightly. Where users previously called

F, ∇ₓF = F_and_∇ₓF(args...)

which returned two functions (F is the tendencies of the tracers and ∇ₓF its Jacobian w.r.t the tracers), now users should call

F = AIBECSFunction(args...)

which returns only F. The Jacobian is still accessible (by calling F.jac(args...), following SciML syntax) but is not exposed to the users who generally don't need to directly use it. The output of AIBECSFunction(args...) is now cleaner and in line with the SciML ecosystem.

For paper.md:

  • L11-15: It might be good to provide one or two examples for both simple and complicated models.

We have added the Archer et al. (2000) 2-box model example for the simple case and the MITgcm for an example of a complicated model.

  • L16: It would be good to point out that AIBECS.jl only resolves steady states.

We have added "steady-state" here (and multiple other places following @dankelley's comments).

  • L31: It is worth mentioning that minimizing model-observation mismatches may cause overfitting.

We have followed @dankelley's suggestion here and revised the text to simply say "may improve".

  • L73: Taburet_etal_OcnSci_2019 citation is broken.

This citation has been fixed.

  • L102-119: I think there are too many citations, only one or two for each kind of model is enough.

This list has been cut down. (See response to the similar comment made by @dankelley above.)

kthyng commented 2 years ago

@zhenwu0728 can you state here if you consider your review satisfactorily addressed?

zhenwu0728 commented 2 years ago

Hi @kthyng, I think the authors did a good job. I realized that translating the examples to Pluto requires a lot of effort and the authors can do it at their own pace. The current examples are also good enough.

kthyng commented 2 years ago

@zhenwu0728 Thank you!

It looks like both reviews have been completed, so we can proceed with publication.

kthyng commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

kthyng commented 2 years ago

@briochemc can you archive your software in a place like Zenodo and report back the doi here? Also please update the metadata in the archive so the author list and title exactly match your JOSS paper. Also, please state here the software version you would like associated with your JOSS paper — you'd probably want to make a new tag/release of the software also.

kthyng commented 2 years ago

@briochemc The paper looks good, but:

briochemc commented 2 years ago

I am almost done revising code/paper according to @kthyng. I still need to check the new PDF as built by Whedon and if all is good [update: Done] then tag the release with the JuliaRegistrator bot [update: Done].

briochemc commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

briochemc commented 2 years ago

@kthyng, I am done with the checklist above! Anything else I should do?

kthyng commented 2 years ago

@briochemc Sorry for my delay, I had the past few weeks off! Everything is looking ready to go now!

kthyng commented 2 years ago

@whedon set v0.11.2 as version