Joss-reviews: [REVIEW]: OceanSpy: A Python package to facilitate ocean model data analysis and visualization

[whedon]

Submitting author: @malmans2 (Mattia Almansi)
Repository: https://github.com/malmans2/oceanspy
Version: 0.1.0
Editor: @kthyng
Reviewer: @tomchor, @platipodium
Archive: 10.5281/zenodo.3270646

Status

Status badge code:

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

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

@tomchor & @platipodium , 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 try and complete your review in the next two weeks ✨

Review checklist for @tomchor

Conflict of interest

• [x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• [x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• [x] Repository: Is the source code for this software available at the repository url?
• [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
• [x] Version: 0.1.0
• [x] Authorship: Has the submitting author (@malmans2) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?
• [x] Functionality: Have the functional claims of the software been confirmed?
• [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• [x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
• [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

• [x] Authors: Does the paper.md file include a list of authors with their affiliations?
• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @platipodium

Conflict of interest

• [x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• [x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• [x] Repository: Is the source code for this software available at the repository url?
• [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
• [x] Version: 0.1.0
• [x] Authorship: Has the submitting author (@malmans2) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• [ ] Installation: Does installation proceed as outlined in the documentation?
• [ ] Functionality: Have the functional claims of the software been confirmed?
• [ ] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• [ ] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
• [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

• [x] Authors: Does the paper.md file include a list of authors with their affiliations?
• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [ ] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

[whedon]

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

1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

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

@whedon commands

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

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

[platipodium]

Opened https://github.com/malmans2/oceanspy/issues/107 to complete doi referencing in @malmans2 paper/paper.bib

[malmans2]

@whedon commands

[whedon]

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

[malmans2]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

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

[tomchor]

@kthyng I'm not 100% sure if this is the appropriate behavior, but I'm pasting my review below. Most of the comments are general so I figured it wouldn't be appropriate to create an issue in the target repo for them. Please let me know if there's anything else I should do.

Best
Tomas.

Overall impressions

This is a nice package and is likely to be helpful to scientists who do ocean modelling. The package, from what I understood, is a collection of wrappers specific to oceanography around already-existing packages. I recommend publication with minor revisions based on the comments below.

Comments on documentation checklist

Statement of need: the target audience is very well stated, but in my opinion the problems the package aims to solve could be better stated. The paper itself has those problems/needs very well explained however.

Installation instructions: although the installation instructions are very clear, there is no explicitly stated list of dependencies. Instead the dependencies are implicitly listed in a conda install command.

Automated tests: there were no automated tests that I could find in the package.

Comments on SciServer

The SciServer part in the tutorial was a bit confusing to me. This only a suggestion, but I think It would also be good to state the differences of working from within SciServer. (I know that there is a paragraph in the beginning on the docs trying to clarify the SciServer/standalone options, but I'm not sure if everyone is familiar with SciServer.) The flowchart, for example, could be one of the first things in the documentation (or at least in the getting started portion). Instead it is given a separate section with no context whatsoever.

Furthermore, the steps to start a SciServer didn't quite work for me. After clicking on compute, there was no option to Run Existing Notebook. I suspect they are outdated. It was very straightforward to figure out by myself the correct steps, but it would still be nice to have the correct instructions.

Specific comments

• In some places (generally towards the ends of the tutorial sections) the comments tend to become scarce. An example are cells [7] through [9] in the Kogur case. A little explanation in these cases would benefit users.
• Before cell [13] in https://oceanspy.readthedocs.io/en/latest/Tutorial.html the comment should say "while this syntax adds the new variable to od_drop"
• I think it is really nice that in the paper (and also in the autogen docs) the calculations done by the compute module are mathematically defined. However, as a scientist, it would be nice to know which numerical scheme is used to perform those computations. I assume it is the default functionality of xarray, which uses specific numpy functions. A simple reference to the xarray function would suffice (to avoid having to go through code to figure it out).

[malmans2]

Thanks @tomchor! We are going to open a new pull request to address your comments. It looks we can address everything by improving the documentation (we do have automated tests that are covering 98% of the code but they are only mentioned in the contributing section. We will probably add test instructions in the installation section to make it more clear).

@platipodium @kthyng, can we just merge the pull request when we're done, or should we wait for the other review? If we don't merge the pull request, the documentation needs to be compiled locally in order to see the changes.

[platipodium]

I am happy with merging the PR, us rebuilding docs is part of the job description I guess.

[malmans2]

@tomchor, thanks for your quick and helpful review! I think we've been able to address all your points. The new documentation is already online.

Statement of need: the target audience is very well stated, but in my opinion the problems the package aims to solve could be better stated. The paper itself has those problems/needs very well explained however.

We edited the github README, which is also the front page of the documentation: https://oceanspy.readthedocs.io

Installation instructions: although the installation instructions are very clear, there is no explicitly stated list of dependencies. Instead the dependencies are implicitly listed in a conda install command.

Required and optional dependencies are now explicitly stated here.

Automated tests: there were no automated tests that I could find in the package.

We have automated tests and they cover 98% of the code. Instructions on how to run and implement tests are available here. We also added a section in the installation page to make it more clear. You can check continuous integration and code coverage by clicking on the stickers in the front page.

The SciServer part in the tutorial was a bit confusing to me. This only a suggestion, but I think It would also be good to state the differences of working from within SciServer. (I know that there is a paragraph in the beginning on the docs trying to clarify the SciServer/standalone options, but I'm not sure if everyone is familiar with SciServer.) The flowchart, for example, could be one of the first things in the documentation (or at least in the getting started portion). Instead it is given a separate section with no context whatsoever.

We revised the quick start and SciServer sections to make it more clear, and the flowchart is now in the quick start section. We also added in bold "How to use OceanSpy?" in the front page.

Furthermore, the steps to start a SciServer didn't quite work for me. After clicking on compute, there was no option to Run Existing Notebook. I suspect they are outdated. It was very straightforward to figure out by myself the correct steps, but it would still be nice to have the correct instructions.

We had a typo in the SciServer Job section that originated the confusion. Point 3 now says: click on compute Jobs, which is the SciServer app that allows to run jobs asynchronously. The steps in quick-start, which use the interactive domain, should be up-to-date.

In some places (generally towards the ends of the tutorial sections) the comments tend to become scarce. An example are cells [7] through [9] in the Kogur case. A little explanation in these cases would benefit users.

All code cells are now preceded by markdown comments.

Before cell [13] in https://oceanspy.readthedocs.io/en/latest/Tutorial.html the comment should say "while this syntax adds the new variable to od_drop"

Fixed!

I think it is really nice that in the paper (and also in the autogen docs) the calculations done by the compute module are mathematically defined. However, as a scientist, it would be nice to know which numerical scheme is used to perform those computations. I assume it is the default functionality of xarray, which uses specific numpy functions. A simple reference to the xarray function would suffice (to avoid having to go through code to figure it out).

OceanSpy is mainly targeting models that use finite volume methods. We tested OceanSpy using MITgcm simulations, so we used the MITgcm schemes as reference.
Here is an example: To compute the vertical component of the relative vorticity we use the OceanSpy's curl function. The vorticity computed by the model online and by OceanSpy agree to machine precision. OceanSpy's curl has a "Numerical Method" reference that points to the MITgcm documentation. Since OceanSpy's vertical_relative_vorticity uses the curl function, curl is listed under "See Also" of vertical_relative_vorticity (it's also a link).

[tomchor]

@malmans2 thanks for the reply. I just have one more comment before I consider the review finished.

OceanSpy is mainly targeting models that use finite volume methods. We tested OceanSpy using MITgcm simulations, so we used the MITgcm schemes as reference.
Here is an example: To compute the vertical component of the relative vorticity we use the OceanSpy's curl function. The vorticity computed by the model online and by OceanSpy agree to machine precision. OceanSpy's curl has a "Numerical Method" reference that points to the MITgcm documentation. Since OceanSpy's vertical_relative_vorticity uses the curl function, curl is listed under "See Also" of vertical_relative_vorticity (it's also a link).

I think it's a good idea to choose your numerical schemes in a way that agrees with the model. The downside is that you have to pick one model (in this case MITgcm), which makes the package less general and not appropriate for some other models depending on their numerical scheme.

This is important information and should be mentioned somewhere that's easy to find in the documentation. At the moment, I can't find this information anywhere. I had noticed the link to MITgcm in your computing functions, but since it points to the notation page, it wasn't clear to me at all that you had mimicked MITgcm's schemes.

Thus, my last point is simply to make it clear that the package (at least the computing part) is focused on and tailored towards MITgcm results.

Cheers!

[malmans2]

@tomchor, that's a good point! We added a highlighted note in the front page.

[kthyng]

Thanks for everyone's effort so far! Sorry for my slow response; I've been traveling.

@tomchor Are there any lingering issues you'd like to see addressed? I see one checkbox still unchecked.

@platipodium How are things from your perspective?

[tomchor]

@kthyng I apologize, I forgot to update my checkboxes. There are no more issues to be addressed. I believe now I've ticked every checkbox I had to! :)

Cheers

[platipodium]

I have so far failed to successfully install the software on my local system, neither via conda forge nor on my system python installation, that's why I did not check functionality.

[platipodium]

Whoa, what happened, I seem not to be able to tick my boxes above anymore ...

[malmans2]

@platipodium could you please tell me a little more about the issues you are encountering?
In my experience the following commands usually solve all problems:

$ conda config --set channel_priority strict
$ conda config --prepend channels conda-forge

They basically just edit you .condarc, which should look like:

channel_priority: strict
channels:
  - conda-forge
  - defaults

If your .condarc has been edited before and looks different, or if you are using a very old version of conda, this could be another source of problems.

[malmans2]

@platipodium, just checking if you are still stuck on the installation. I'm able to install on our machines and the continuous integration is also successfully creating the environment, so I would need more details to get it fixed (e.g., log, OS, ...).

[platipodium]

Thanks for checking on me. I have finally been able to overcome the issues I faced, all of which were related to permissions gone wrong in my conda installation. Nothing to worry about in your package. But something we too often forget with CI and package management, that also those systems are far from perfect and working for everyone. I have not yet gone through the steps to replicating your demo functionality on my local system, but that will be done soon. Sorry for the delay.

[malmans2]

Good news, thanks! I was worried about problems related to the OS since we mainly test and develop on linux machines.

[platipodium]

@malmans2 I have now tested the claimed funtcionality, also on my local system, @kthyng I still can't tick my boxes (but I would if I could). So I guess the review is complete.

[kthyng]

@platipodium I haven't heard about that happening before, but thank you for clearly stating that your review is complete.

[kthyng]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

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

[kthyng]

@whedon check references

[whedon]

Attempting to check references...

[whedon]

```Reference check summary:

OK DOIs

• 10.5334/jors.148 is OK
• 10.1145/2949689.2949700 is OK
• 10.5281/zenodo.826926 is OK
• 10.1017/9781139015721 is OK

MISSING DOIs

• None

INVALID DOIs

• None
  ```

[kthyng]

Hi @malmans2! Can you clarify which ocean models OceanSpy will work with in your paper? After that, please make a Zenodo archive, and report the DOI in this thread.

[malmans2]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

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

[malmans2]

@whedon check references

[whedon]

Attempting to check references...

[whedon]

```Reference check summary:

OK DOIs

• 10.5334/jors.148 is OK
• 10.1145/2949689.2949700 is OK
• 10.5281/zenodo.826926 is OK
• 10.1017/9781139015721 is OK
• 10.1029/96JC02775 is OK

MISSING DOIs

• None

INVALID DOIs

• None
  ```

[malmans2]

Hi @malmans2! Can you clarify which ocean models OceanSpy will work with in your paper? After that, please make a Zenodo archive, and report the DOI in this thread.

Hi @kthyng, I think we're all set. We've updated the paper and added a reference. Before creating the doi I've released v0.1.0, so I think we need to inform whedon.
Here is the doi: 10.5281/zenodo.3270646

[kthyng]

@malmans2 excellent! You may be getting some notes in the future as I encourage people I know to try out this package!

I'll have whedon update several things and feel free to let me know if you think I've missed something.

First though, do you want to keep the version as 0.1 instead of 1? Publishing this paper could be a good reason to bump up the version all the way to a whole number :)

[kthyng]

(obviously completely up to you, just a suggestion)

[malmans2]

Thanks a lot for the feedback and helping disseminate OceanSpy!
I would keep 0.1 for now. I think we have a few tasks before version 1:

• Get more feedback and contributions from other groups (both observational and modeling)
• Test/adapt OceanSpy on different models
• Fully implement the particle code
• Upload LLC4320 on SciServer and test/improve OceanSpy's performances on high-res global datasets.

[kthyng]

@whedon set 10.5281/zenodo.3270646 as archive

[whedon]

OK. 10.5281/zenodo.3270646 is the archive.

[kthyng]

@whedon set 0.1.0 as version

[whedon]

OK. 0.1.0 is the version.

[kthyng]

@openjournals/joss-eics This paper is ready for acceptance! Nice work @malmans2 and thank you to @platipodium and @tomchor for your reviews!

[labarba]

@whedon accept

[whedon]

Attempting dry run of processing paper acceptance...

[whedon]

```Reference check summary:

OK DOIs

• 10.5334/jors.148 is OK
• 10.1145/2949689.2949700 is OK
• 10.5281/zenodo.826926 is OK
• 10.1017/9781139015721 is OK
• 10.1029/96JC02775 is OK

MISSING DOIs

• None

INVALID DOIs

• None
  ```

[whedon]

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/822

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/822, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.
@whedon accept deposit=true

[labarba]

@whedon accept deposit=true

[whedon]

Doing it live! Attempting automated processing of paper acceptance...

[whedon]

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

[whedon]

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/823
2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01506
3. If everything looks good, then close this review issue.

4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

   Any issues? notify your editorial technical team...

[labarba]

Congratulations, @malmans2, your JOSS paper is published! 🚀

Big thanks to our editor: @kthyng, and the reviewers: @tomchor, @platipodium 🙏

[whedon]

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.01506/status.svg)](https://doi.org/10.21105/joss.01506)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01506">
  <img src="http://joss.theoj.org/papers/10.21105/joss.01506/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.01506/status.svg
   :target: https://doi.org/10.21105/joss.01506

This is how it will look in your documentation:

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

• Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
• Making a small donation to support our running costs here: https://numfocus.salsalabs.org/donate-to-joss

[malmans2]

Thanks @platipodium and @tomchor for the constructive comments! @kthyng, I loved the whole JOSS process. Let me know if you'll need me as reviewer in the future!

[kthyng]

@malmans2 Definitely will be asking you for reviews! I see you're already on the list. :)