-
-
Notifications
You must be signed in to change notification settings - Fork 38
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]: PolarToolkit: Python Tools for Convenient, Reproducible, and Open Polar Science #6502
Comments
Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks. For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
Software report:
Commit count by author:
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: ✅ License found: |
Review checklist for @PennyHowConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Review checklist for @JessicaS11Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
@JessicaS11 @PennyHow just a friendly reminder not to forget this review |
I am struggling to install PolarToolkit and have been trying to follow the instructions for constructing it with conda in Python 3.9, 3.10, 3.11 and 3.12 environments. Mainly it is unable to solve the environment with the stated dependencies and packages with C bindings. @mdtanker, could you please provide an environment.yml file so that I can construct an environment and install polartoolkits from a local copy of your repo? I.e.
In terms of the paper and documentation, I think these are generally looking good. I've made some suggestions and put them into a PR here. The main points are:
I can provide a review on the package functionality once I can run polartoolkit. |
@PennyHow sorry that there are issues with the install! I used to encounter lots of issues due to the C dependencies but thought all of that had been fixed. I guess not... GitHub is not letting me attach a Please let me know if that works. |
It works! Thank you. |
Hello @mdtanker! Great toolkit. Below please find my review comments, notwithstanding mdtanker/polartoolkit#193, mdtanker/polartoolkit#194, mdtanker/polartoolkit#62, and agreement with review comments from @PennyHow on adding more information to the documentation. DocumentationI really like all the invitations for contributions, including reminders and notes that it's okay if your submission isn't perfect or you don't know how to use all the tools.
Do you have any examples of a scientific workflow (perhaps as part of a tutorial or publication) containing one of the figures generated by this software?
For updates (or new) notebooks, should contributors run them locally and push with outputs, or clear all outputs? Software PaperThere's a link typo (to the old repo) on line 38.
The summary is well written, though it suggests the current functionality enables all "Polar" research while in reality it is still limited to Antarctica, with plans to expand to the Arctic/Greenland (yay!). This technicality is important from a user perspective and also because expanding the current package to the north pole will include substantial additions that are not captured in the software release with the current paper. It would also be helpful for more details in the paper (and documentation) around what types of datasets are available, and what types of polar research they are enabling. For instance, it appears that most (all?) of the source datasets are gridded.
Could you please elaborate on how this fits into the broader tool landscape? For instance, as noted in the documentation, the fetch module uses icepack's EarthDataDownloader class. |
Thanks for a nice submission, @mdtanker! Generally I feel that this is a great package with good entry-level functionality for beginners in programming, and also handy tools for seasoned cryospheric coders to quickly generate publication-ready figures. The documentation and package structuring reflects these two target user types, with thorough documentation and tutorials. I have two major comments that need to be addressed before accepting, along with some minor comments: The functionality is limited to AntarcticaThe term "polar" places an expectation on the package being usable with Antarctic and Arctic datasets. Currently functionality only accomodates datasets for Antarctica. You say that ideally you would like polartoolkit to develop further for use with data from Greenland and the Arctic. On the readthedocs home page, it is stated that this is happening "soon". When is "soon" and would this be within the timeframe of this review process? Whilst I do not need to see a full implementation of these Greenland/Arctic developments, I think there should be certain changes in the naming conventions and package structuring so that it is easy to foresee how Greenland/Arctic datasets and analysis could be incorporated. For example, I see that the names of the currently available datasets are generically named and not place-specific:
These should be changed accordingly so that Greenland/Arctic dataset names could be easily incorporated, i.e. basal_melt >> antarctic_basal_melt. If you think it is possible, I would also like to request one Greenland/Arctic example be developed and placed in the tutorials/gallery to demonstrate the viability of this future development. The installation needs to be more straightforward for beginner usersThe extensive list of package dependencies mean that it takes a long time for mamba/conda to solve the environment with installation. Even the Binder online build did not work for me as it cannot solve the environment in a timely manner. As this package is partially aimed at beginners in programming, the set-up needs to be simple otherwise it is not accessible for these users. Whilst the installation troubleshooting documentation is good, I think that the package should be modified to make the installation easier. You can solve this problem however you think is best, whether it be changing the package distribution itself, or adding more extensive installation guidance. Suggestion: I would look at the number of dependencies and refine them by trying to find common functionality between them. And I would also look at alternatives to geopandas, cartopy and pygmt that do not have C bindings (as this is may resolve the issues with the online Binder build). Minor commentsFunctionality
Installation
Documentation
Software paper
Other
|
@mdtanker could you please let us know when you plan to address the reviewers' suggestions/issues? |
Hi all, thank you all for the prompt and thorough reviews! I really appreciate all the time you have put in. Apologies for not responding sooner, but I've been trying to address many of the suggestions before doing a line-by-line response. I've made lots of commits in the past two weeks to the PR: joss-edits. Some of these suggestions will take some more time to correctly implement, but I anticipate being done within 2-3 weeks. The major things left for me to do are add all the available datasets to a gallery, add a few more Greenland-specific datasets, and try and reduce the dependencies and size of the repo. Once I have accomplished most of these tasks, I will address all your comments here 😃 |
Another update: this turned into a slightly bigger task than I had anticipated. Adding support for Greenland and the Arctic required some pretty significant refactoring of the code, but I think it is done now. Remaining tasks to do:
I should be able to get these done soon and give line-by-line responses to the suggestions. Thanks again for all of your patience with this! |
@editorialbot set v0.5.1 as version |
Done! version is now v0.5.1 |
@editorialbot recommend-accept |
|
|
👋 @openjournals/ese-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#5782, then you can now move forward with accepting the submission by compiling again with the command |
@editorialbot accept |
I'm sorry @mdtanker, I'm afraid I can't do that. That's something only eics are allowed to do. |
Looks good to me! |
Hi! I'll take over now as Track Associate Editor in Chief to do some final submission editing checks. After these checks are complete, I will publish your submission!
|
@mdtanker I am delighted by your histogram colorbar! I would love to see that available in a more general toolbox, something like matplotlib! Might you consider that? |
Yes I'd love to see that implemented in matplotlib! I'll have a look sometime at their code and how to implement it. I find it so useful to show the distribution! |
Sorry I didn't dig deep enough to see whose code it was. But regardless, very cool, along with the rest of the package! Everything looks ready to go! |
@editorialbot accept |
|
Ensure proper citation by uploading a plain text CITATION.cff file to the default branch of your repository. If using GitHub, a Cite this repository menu will appear in the About section, containing both APA and BibTeX formats. When exported to Zotero using a browser plugin, Zotero will automatically create an entry using the information contained in the .cff file. You can copy the contents for your CITATION.cff file here: CITATION.cff
If the repository is not hosted on GitHub, a .cff file can still be uploaded to set your preferred citation. Users will be able to manually copy and paste the citation. |
🐘🐘🐘 👉 Toot for this paper 👈 🐘🐘🐘 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
Congratulations on your new publication @mdtanker! Many thanks to editor @hugoledoux and to reviewers @PennyHow and @JessicaS11 for your time, hard work, and expertise!! JOSS wouldn't be able to function nor succeed without your efforts. @mdtanker If you're interested in joining JOSS as a reviewer, please sign up at https://reviewers.joss.theoj.org/! |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! The 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:
|
Wohoo! Thanks for all the time and energy everyone! I enjoyed this review process and think PolarToolkit is much better due to it! I've signed up to be a reviewer 🙂 |
@mdtanker, I think you are a great addition to the team of reviewers behind JOSS.Welcome! |
Submitting author: @mdtanker (Matt Tankersley)
Repository: https://github.com/mdtanker/polartoolkit
Branch with paper.md (empty if default branch): JOSS_paper
Version: v0.5.1
Editor: @hugoledoux
Reviewers: @PennyHow, @JessicaS11
Archive: 10.5281/zenodo.13218540
Status
Status badge code:
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
@PennyHow & @JessicaS11, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review.
First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @hugoledoux 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 ✨
Checklists
📝 Checklist for @PennyHow
📝 Checklist for @JessicaS11
The text was updated successfully, but these errors were encountered: