Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW]: OceanSpy: A Python package to facilitate ocean model data analysis and visualization #1506

Closed
whedon opened this issue Jun 13, 2019 · 59 comments

Comments

Projects
None yet
6 participants
@whedon
Copy link
Collaborator

commented Jun 13, 2019

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

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

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: 0.1.0
  • 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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • 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?
  • 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

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • 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)?

Review checklist for @platipodium

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: 0.1.0
  • 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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • 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?
  • 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

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • 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

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

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 🎉.

⭐️ Important ⭐️

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
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

@platipodium

This comment has been minimized.

Copy link

commented Jun 14, 2019

Opened malmans2/oceanspy#107 to complete doi referencing in @malmans2 paper/paper.bib

@malmans2

This comment has been minimized.

Copy link

commented Jun 14, 2019

@whedon commands

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

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

This comment has been minimized.

Copy link

commented Jun 14, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

@tomchor

This comment has been minimized.

Copy link

commented Jun 17, 2019

@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 tutorial) 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 malmans2 referenced this issue Jun 17, 2019

Closed

Review from tomchor #109

7 of 7 tasks complete
@malmans2

This comment has been minimized.

Copy link

commented Jun 17, 2019

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

This comment has been minimized.

Copy link

commented Jun 18, 2019

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

@malmans2

This comment has been minimized.

Copy link

commented Jun 19, 2019

@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 tutorial) 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

This comment has been minimized.

Copy link

commented Jun 19, 2019

@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

This comment has been minimized.

Copy link

commented Jun 20, 2019

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

@kthyng

This comment has been minimized.

Copy link

commented Jun 25, 2019

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

This comment has been minimized.

Copy link

commented Jun 25, 2019

@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

This comment has been minimized.

Copy link

commented Jun 25, 2019

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

This comment has been minimized.

Copy link

commented Jun 25, 2019

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

@malmans2

This comment has been minimized.

Copy link

commented Jun 25, 2019

@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

This comment has been minimized.

Copy link

commented Jul 3, 2019

@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

This comment has been minimized.

Copy link

commented Jul 3, 2019

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

This comment has been minimized.

Copy link

commented Jul 3, 2019

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

@kthyng

This comment has been minimized.

Copy link

commented Jul 5, 2019

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

This comment has been minimized.

Copy link

commented Jul 6, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 6, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 6, 2019

@malmans2

This comment has been minimized.

Copy link

commented Jul 6, 2019

@whedon check references

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 6, 2019

Attempting to check references...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 6, 2019


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

This comment has been minimized.

Copy link

commented Jul 6, 2019

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

This comment has been minimized.

Copy link

commented Jul 6, 2019

@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

This comment has been minimized.

Copy link

commented Jul 6, 2019

(obviously completely up to you, just a suggestion)

@malmans2

This comment has been minimized.

Copy link

commented Jul 6, 2019

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

This comment has been minimized.

Copy link

commented Jul 6, 2019

@whedon set 10.5281/zenodo.3270646 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 6, 2019

OK. 10.5281/zenodo.3270646 is the archive.

@kthyng

This comment has been minimized.

Copy link

commented Jul 6, 2019

@whedon set 0.1.0 as version

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 6, 2019

OK. 0.1.0 is the version.

@kthyng kthyng added the accepted label Jul 6, 2019

@kthyng

This comment has been minimized.

Copy link

commented Jul 6, 2019

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

@labarba

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

Attempting dry run of processing paper acceptance...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019


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

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

Check final proof 👉 openjournals/joss-papers#822

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#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

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

@whedon accept deposit=true

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

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

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

🚨🚨🚨 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 👉 openjournals/joss-papers#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

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

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

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

@labarba labarba closed this Jul 7, 2019

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

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:

DOI

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:

@malmans2

This comment has been minimized.

Copy link

commented Jul 7, 2019

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

This comment has been minimized.

Copy link

commented Jul 7, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.