pyOpenSci / software-submission

Submit your package for review by pyOpenSci here! If you have questions please post them here: https://pyopensci.discourse.group/
89 stars 33 forks source link

xnemogcm #155

Closed rcaneill closed 1 month ago

rcaneill commented 5 months ago

as discussed in #154 I make a full submission

Submitting Author: Romain Caneill (@rcaneill)
All current maintainers: (@rcaneill) Package Name: xnemogcm One-Line Description of Package: Interface to open NEMO global circulation model output dataset with xarray and create a xgcm grid. Repository Link: https://github.com/rcaneill/xnemogcm/ Version submitted: 0.4.2 Editor: @ocefpaf Reviewer 1: @paigem
Reviewer 2: @callumrollo
Archive: DOI Version accepted: 0.4.3 JOSS DOI: N/A Date accepted (month/day/year): 04/02/2024


Code of Conduct & Commitment to Maintain Package

Description

xnemogcm is an interface to open NEMO ocean global circulation model output dataset and create a xgcm grid. NEMO 3.6, 4.0, and 4.2.0 are tested and supported. It can handle large simulations, is aware of meshgrid files, and makes it easy to handle the netCDF outputs od NEMO.

Scope

Domain Specific & Community Partnerships

- [ ] Geospatial
- [ ] Education
- [x] Pangeo

Community Partnerships

If your package is associated with an existing community please check below:

xnemogcm extracts the NEMO output and adds metadata (+ do some other things as change coordinate names, etc), and produces new xarray datasets. It is thus data processing.

The target audience is anyone working with NEMO outputs and python. The scientific applications are any analyse that one want to do with NEMO outputs.

Yes, xorca. My package differs as 1) it is more recent, updated to work with newer versions of NEMO, and 2) it uses a very different method to sort variables on the proper grid point (center, face, etc) (cf https://xgcm.readthedocs.io/en/latest/grids.html). xorca uses hardcoded variables, while xnemogcm uses attributes (either output directly by NEMO, or they can also be given while calling the processing functions).

I made a pre-submission enquiry, issue #154

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

Publication Options

JOSS Checks - [ ] The package has an **obvious research application** according to JOSS's definition in their [submission requirements][JossSubmissionRequirements]. Be aware that completing the pyOpenSci review process **does not** guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS. - [ ] The package is not a "minor utility" as defined by JOSS's [submission requirements][JossSubmissionRequirements]: "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria. - [ ] The package contains a `paper.md` matching [JOSS's requirements][JossPaperRequirements] with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: *Note: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.*

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

Confirm each of the following by checking the box.

Please fill out our survey

P.S. Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

The editor template can be found here.

The review template can be found here.

ocefpaf commented 5 months ago

Editor in Chief checks

Hi there! Thank you for submitting your package for pyOpenSci review. Below are the basic checks that your package needs to pass to begin our review. If some of these are missing, we will ask you to work on them before the review process begins.

Please check our Python packaging guide for more information on the elements below.



Editor comments

rcaneill commented 5 months ago

Hi @ocefpaf, thanks for taking care of this review :)

I can directly mention that some requirements are not fulfilled:

As far as I can say, the other requirements should be fulfilled.

ocefpaf commented 5 months ago

That's awesome! Here are a few minor comments/questions I have after the first pass:

rcaneill commented 5 months ago

That's awesome! Here are a few minor comments/questions I have after the first pass:

  • You mentioned that the GH repo is a mirror of a Gitlab one. IMO this dual repo usually leads to abandoned PRs in one place and/or stale code somewhere. Let alone the confusion to those finding the Software via search engines. I'm not against this setup and sometimes there are strong reasons to do so. However, if that can be avoided, it usually provides a smoother experience to all.

Maybe the README is not so clear, the github is mirrored to gitlab (not from gitlab). The reason is that I do not want to force anyone to use github. But I could remove the sentence and keep my email address only.

  • Are you publishing the docs somewhere? I could not find them. I understand that the API docs are not ready but you do have some examples that can be the first form of user facing docs.

No, I did not publish the doc anywhere. I started all the readthedoc machinery, but realised that having only these few examples was sufficient. As this packages has been developed only by me, I also had a lot to learn and a proper website for the doc was not urgent.

I can work on a proper website for the doc during this review process. I do not aim to change the content of the examples (unless I get comments from the reviewers), so I think the review can start without this implemented yet.

  • I started checking the boxes above. Please let me know if I missed one that is already done.
ocefpaf commented 5 months ago

Maybe the README is not so clear, the github is mirrored to gitlab (not from gitlab). The reason is that I do not want to force anyone to use github. But I could remove the sentence and keep my email address only.

I understand. By making a choice we always end up making some folks unhappy. In my experience, mirroring like that always brings headaches in the long run. With that said, this is not important for your application here and you don't need to change that for PyOS.

I can work on a proper website for the doc during this review process. I do not aim to change the content of the examples (unless I get comments from the reviewers), so I think the review can start without this implemented yet.

@rcaneill we usually only start the review after all those points are checked. The reason is b/c the reviewers will read those docs and give you some feedback. Let me know if you need help setting docs, examples, etc in your repo. Once we are done I'll find two reviewers for your project and start the review process.

rcaneill commented 5 months ago

@rcaneill we usually only start the review after all those points are checked. The reason is b/c the reviewers will read those docs and give you some feedback. Let me know if you need help setting docs, examples, etc in your repo. Once we are done I'll find two reviewers for your project and start the review process.

Ok I understand, I will try to check all boxes in the following weeks!

rcaneill commented 4 months ago

Hi @ocefpaf , I updated the xnemogcm github repository, everything should be checked now! :D

ocefpaf commented 4 months ago

@rcaneill there seems to be a documentation build problem in page https://xnemogcm.readthedocs.io/en/latest/examples/recombing_mesh_mask_domain_cfg/

Also, some of your syntax there is not Windows friendly and users that copy-n-paste may find themselves with errors. Those are not blockers for us to continue but I wanted to give that feedback here before I forget.

We are now looking for 2 reviewers to start the process. Thank you!

rcaneill commented 4 months ago

@rcaneill there seems to be a documentation build problem in page https://xnemogcm.readthedocs.io/en/latest/examples/recombing_mesh_mask_domain_cfg/

thanks for catching this, it is now corrected.

Also, some of your syntax there is not Windows friendly and users that copy-n-paste may find themselves with errors. Those are not blockers for us to continue but I wanted to give that feedback here before I forget.

Are you speaking about the ls commands in the notebooks? If yes, I can change and use python instead.

ocefpaf commented 4 months ago

Are you speaking about the ls commands in the notebooks? If yes, I can change and use python instead.

That and I believe some of the paths may not be parsed correctly. I don't have a Windows machine to test in Path is smart enough to convert / to \ for us.

rcaneill commented 4 months ago

pathlib.Path is handling the windows conversion: https://stackoverflow.com/questions/2953834/how-should-i-write-a-windows-path-in-a-python-string-literal/68234511#68234511

For the ls, I changed to os.listdir in https://github.com/rcaneill/xnemogcm/pull/80

ocefpaf commented 4 months ago

:wave: Hi Callum Rollo and Paige Martin! Thank you for volunteering to review for pyOpenSci!

The following resources will help you complete your review:

  1. Here is the reviewers guide. This guide contains all of the steps and information needed to complete your review.
  2. Here is the review template that you will need to fill out and submit here as a comment, once your review is complete.

Please get in touch with any questions or concerns! Your review is due: March 5th, 2024 (3 weeks). Please let us know if you need more time.


Reviewers: @callumrollo and @paigem Due date: 2024-02-29

PS: You can comment, open issue, and/or PR directly in the xnemogcm for this review. Just make sure to cross-reference them here.

callumrollo commented 4 months ago

Hej Romain, I'm starting my review today. The package looks really good!

I see you have an Issue in to rename master >> main. Perhaps now is a good time to do that before I queue up a couple of small PRs https://github.com/rcaneill/xnemogcm/issues/72

callumrollo commented 4 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing:

5 hours


Review Comments

rcaneill commented 4 months ago

Hej Romain, I'm starting my review today. The package looks really good!

I see you have an Issue in to rename master >> main. Perhaps now is a good time to do that before I queue up a couple of small PRs rcaneill/xnemogcm#72

Hi Calum, thanks for your time :), I updated the name of the default branch to main

rcaneill commented 4 months ago

Thanks @callumrollo for all the PR and improvements.

@ocefpaf am I supposed to check / merge the PR on the fly, or wait for the end of the review?

ocefpaf commented 4 months ago

@ocefpaf am I supposed to check / merge the PR on the fly, or wait for the end of the review?

Up to you. Sometimes the second reviewer can add upon the first one and waiting makes sense. Sometimes merging/solving everything now makes it easier b/c it reduces the context for the second review.

rcaneill commented 4 months ago

Ok I'll try to merge on the way then

callumrollo commented 3 months ago

I've just about completed my review of the package. It looks to be in good shape! I particularly appreciate the comprehensive test suite. I've just raised one last issue with a missing docstring.

I don't work with nemo data, but using the example data provided I have tested that the functions in xnemogcm work as described. Hopefully @paigem can comment with some more domain specific expertise.

rcaneill commented 3 months ago

Thanks @callumrollo for you time and comments, I really appreciate.

callumrollo commented 3 months ago

If this package isn't being submitted to JOSS I believe my review is complete. Thanks @rcaneill for the speedy response to Issues and PRs

ocefpaf commented 3 months ago

Thanks @callumrollo!

paigem commented 3 months ago

@rcaneill I'm starting my review in the next day! Glad to see that you've been updating in real time with @callumrollo's excellent comments, since it has taken me awhile to get to my review. This is my first PyOpenSci review, so just a heads up that I may work a bit slowly (over the course of a couple days) as I learn the ropes.

@callumrollo I also have not used NEMO data before, but I have used a lot of climate model data in the past, so I'll do my best to provide domain-specific knowledge! Thanks for being prompt and thorough with your review - you're making my job slightly easier. 😉

rcaneill commented 3 months ago

Thanks @paigem for your time :), I'll also try to be quick answering your questions if you have some

paigem commented 3 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 7 hours


Review Comments

This is a nice, relatively light-weight package that connects the NEMO community to the broader xarray and xgcm communities. The is a package that I think will be very useful to the ocean science community. Thanks @rcaneill for creating this package!

With the exception of the portion of the review template that has to do with JOSS (I've received no indication that a JOSS publication is desired), I have completed my review and approve this package.

I summarize my comments/issues/PRs during the review process below (these have all now been addressed):

paigem commented 3 months ago

@rcaneill My first reaction to this package is that it seems very useful for the scientific community, allowing those familiar with Xarray and xgcm to more easily use and analyze NEMO data. 🎉

I've been focusing on documentation today, and have a couple comments that I put into issues:

rcaneill commented 3 months ago

Thanks @paigem for your comments, these are fair points :)

paigem commented 3 months ago

Thanks for your quick updates @rcaneill - it all looks great!

I'm continuing to go through my review checklist and have a couple questions:

I am continuing through the checklist and will reach out with any further questions.

rcaneill commented 3 months ago

I added CITATION.cff file in https://github.com/rcaneill/xnemogcm/pull/104

paigem commented 3 months ago

@rcaneill I have finished my review! Thanks so much for your quick responses and for being so willing to accept feedback.

@ocefpaf This is to let you know that I have finished my review! Thanks for the opportunity, and let me know if there is anything else I've missed!

rcaneill commented 3 months ago

Thanks @paigem for your time, and for your great suggestions! I really appreciate.

ocefpaf commented 3 months ago

@rcaneill please let me know when you are done parsing the suggestions and please both reviewers for their +1 so we can move this application forward.

rcaneill commented 3 months ago

@ocefpaf I implemented / merged / commented all of their suggestions. So for me, we can move forward :)

ocefpaf commented 3 months ago

@ocefpaf I implemented / merged / commented all of their suggestions. So for me, we can move forward :)

Awesome! @paigem and @callumrollo can I get your +1 before we move to the next stage?

callumrollo commented 2 months ago

@ocefpaf lgtm!

paigem commented 2 months ago

Yes, everything looks great from my end! 😊

ocefpaf commented 2 months ago

@rcaneill I'm moving tomorrow and will be offline for a few days. I'll wrap this one up as soon as I get settle in our new place. If you don't hear from in ~1 week, please ping me again!

ocefpaf commented 2 months ago

🎉 xnemogcm has been approved by pyOpenSci! Thank you Romain Caneill (@rcaneill) for submitting xnemogcm and many thanks to Paige Martin (@paigem) and Callum Rollo (@callumrollo) for reviewing this package! 😸

Author Wrap Up Tasks

There are a few things left to do to wrap up this submission:

Editor Final Checks

Please complete the final steps to wrap up this review. Editor, please do the following:

If you have any feedback for us about the review process please feel free to share it here. We are always looking to improve our process and documentation in the peer-review-guide.

ocefpaf commented 2 months ago

@rcaneill do you need help finalizing the 4 items in https://github.com/pyOpenSci/software-submission/issues/155#issuecomment-2032670632?

Also, if you have some free time can you fill out our maintainers survey at https://forms.gle/BLGVCfUdiJHS5YJY7? @paigem and @callumrollo same for you 😬 . Filling that form will help us improve the process.

ocefpaf commented 2 months ago

BTW, @rcaneill ! We want to invite you and your maintainer team to write a blog post (totally optional) on your package for us to promote your work! if you are interested - here are a few examples of other blog posts:

pandera movingpandas

and here is a markdown example that you could use as a guide when creating your post.

it can even be a tutorial like post that highlights what your package does. then we can share it with people to get the word out about your package.

If you are too busy for this no worries. But if you have time - we'd love to spread the word about your package!

ocefpaf commented 2 months ago

Last but not least, do you (@rcaneill, @paigem, and @callumrollo) would like to be invited to PyOS slack? It will help you share your experience, get in touch with the community, and learn from others.

rcaneill commented 2 months ago

Hi @ocefpaf , I am on parental leave until Monday, I'll take time next week to answer you in details.

ocefpaf commented 2 months ago

Hi @ocefpaf , I am on parental leave until Monday, I'll take time next week to answer you in details.

Enjoy! I'm on forced* parental leave too so that is convenient for both of us 😄

(*Waiting for our day care application to be accepted so we can get the little one in school at the new home.)

callumrollo commented 2 months ago

Last but not least, do you (@rcaneill, @paigem, and @callumrollo) would like to be invited to PyOS slack? It will help you share your experience, get in touch with the community, and learn from others.

sure!

rcaneill commented 2 months ago

The new xnemogcm version is 0.4.3, the doi is: https://doi.org/10.5281/zenodo.10973846

Thanks again @ocefpaf , @callumrollo, and @paigem for the review

rcaneill commented 2 months ago

Last but not least, do you (@rcaneill, @paigem, and @callumrollo) would like to be invited to PyOS slack? It will help you share your experience, get in touch with the community, and learn from others.

Thanks for the invite, but I prefer to not use Slack. I am still available by email for future exchanges.

rcaneill commented 2 months ago

BTW, @rcaneill ! We want to invite you and your maintainer team to write a blog post (totally optional) on your package for us to promote your work! if you are interested - here are a few examples of other blog posts:

pandera movingpandas

and here is a markdown example that you could use as a guide when creating your post.

it can even be a tutorial like post that highlights what your package does. then we can share it with people to get the word out about your package.

If you are too busy for this no worries. But if you have time - we'd love to spread the word about your package!

I will see if I have time for it. If so, should I open a PR on https://github.com/pyOpenSci/pyopensci.github.io?

ocefpaf commented 1 month ago

I will see if I have time for it. If so, should I open a PR on https://github.com/pyOpenSci/pyopensci.github.io?

Yes. You can see an example here.

ocefpaf commented 1 month ago

I'm closing this as complete. Thanks again for the reviewers and for @rcaneill for your patience!