sunpy Review
#147
Comments
|
Thank you kindly for this submission, @nabobalis 🙌 I also appreciate your preparedness beforehand going through our checklists! Editor in Chief checksHi there! Thank you for submitting your package for pyOpenSci Please check our Python packaging guide for more information on the elements
Editor commentsThe link @sunpy/sunpy-developers shows as a 404 for me. My guess is it's a view that needs permissions. Is there an alternative list that is public you can link to? EDIT: for reviewers, Your editor for this review will be @cmarmo |
|
Thank you @isabelizimm for pre-reviewing the submission.
I will list the names in the original post as the team has not changed in a while. |
isabelizimm
added
2/seeking-reviewers
and removed
0/pre-review-checks
New Submission!
labels
|
Hi @nabobalis, nice to meet you, and thanks for submitting to pyOpenSci! |
|
Hi @cmarmo, thank you for handling this review! That is no problem, it gives me a bit more time to check everything is ready on the sunpy side! |
|
Hi @nabobalis, just to let you know that I have found one reviewer, still looking for a second one. |
|
Hi @nabobalis, thank you for your patience! Let's welcome here @Septaris and @nutjob4life who kindly volunteered to review SunPy for pyOpenSci! 👋 Please fill out our pre-review surveyBefore beginning your review, please fill out our pre-review survey. This helps us improve all aspects of our review and better understand our community. No personal data will be shared from this survey - it will only be used in an aggregated format by our Executive Director to improve our processes and programs.
The following resources will help you complete your review:
I'm going to ask reviewers to complete reviews in three weeks, the review for SunPy is then due for December 13th, please let me know if you have any major issue preventing to meet this deadline. Also, please get in touch with any questions or concerns! Thanks again for your commitment! 🙏 |
|
Hi @cmarmo, that is no problem at all. Thank you again for getting this setup and for @Septaris and @nutjob4life taking up the review! Happy holidays to you all! |
|
Hi there ! Thanks for having me for this review. I'm Sonny Lion (@Septaris), research engineer at CNRS and currently working at IJCLab in Orsay. I've been involved in scientific research for a decade, designing, developing, and deploying data processing pipelines and web apps using Python and JavaScript. My participation covers multiple physics projects (particle physics, astrophysics, and energetic transition), as well as dedicated development for lab administration. I also focus on ensuring software quality and teaching best practices at the lab and CNRS. I'm really happy to review |
|
Hi folks; it was the Thanksgiving holiday here in the United States and I'm back to work today. Naturally I've got 497 work emails to get through but I'm looking forward to helping out. Since @Septaris provided some background I will too: I've been with NASA's Jet Propulsion Laboratory for over 20 years now and I work with the Planetary Data System developing mostly Python-based software for handling large-scale atmospheric, geophysical, planetary/plasma, small bodies, and other scientific data. I'm also a contributor to numerous open source Python projects outside of science, and Python packaging is a passion, though so I'll try to be extra thorough in that regard. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Readme file requirements
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.)
UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
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
Final approval (post-review)
Estimated hours spent reviewing: 1.25 hours Review CommentsFirst off, bravo. This is an incredible package that's well-documented and well-assembled. And it's feature-packed! Nice! It's an honor to be able to review it. My notes follow:
$ python3 -m venv sunpy
$ sunpy/bin/pip install --quiet --upgrade pip
$ sunpy/bin/pip install --quiet sunpy==5.0.1
$ sunpy/bin/python
Python 3.11.6 (main, Oct 2 2023, 13:45:54) [Clang 15.0.0 (clang-1500.0.40.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sunpy.map
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/private/tmp/sunpy/lib/python3.11/site-packages/sunpy/map/__init__.py", line 7, in <module>
from sunpy.map.mapbase import *
File "/private/tmp/sunpy/lib/python3.11/site-packages/sunpy/map/mapbase.py", line 15, in <module>
import matplotlib.pyplot as plt
ModuleNotFoundError: No module named 'matplotlib'Maybe the
That's all I got! Thanks for the opportunity to review this. |
|
Thank you @nutjob4life for reviewing the package! If I am allowed to respond to some of the comments, I will do so now, if not, I can delete this after.
Yes this is a good point and I think at this point, only the coordinates module is considered "core" as you found out with map, you need the This actually brings up two very interesting points about installation and how to provide that information. Our readme, only suggests to install with conda and we do not really talk about installing sunpy via pip or "normal" Python at all until you get to https://docs.sunpy.org/en/stable/topic_guide/installation.html We recommend that conda via miniforge is used to install sunpy which is why the readme is like that and that is how we frame it in our install documentation (https://docs.sunpy.org/en/stable/tutorial/installation.html). I wonder what the best approach is, we rather avoid duplicating information into our readme, but the readme does end up at the landing page for the repo and the PyPI page. I don't have any good ideas.
Yeah, I tried to go through it myself and got lost midway.
I will see about adding some examples here and to other util parts. This part of the library was meant to be "developer" facing more than user facing (if one sees a difference there). The idea was that packages that use sunpy or maintainers of sunpy, could call upon these functions for internal code but not as "public" code. But this is not really how it works in practice.
Yeah, our setup.py I think is nearly empty and we plan to move to pyproject.toml but that will be held back as we are currently working on a package template and using cruft to auto re-template our packages. We want to avoid doing that manually.
While we do link to our example gallery in the readme, it's not very obvious.
Luckily we don't 😆
We have talked about adopting black to fix most of our code format but its contentious that we have yet to make a community decision over it. Hopefully that would fix most of these violations.
We did not plan to submit to JOSS as while it has been a long time since we applied. I don't think much has changed that JOSS would accept an updated paper.
Thank you for going through the package, sunpy has really grown and it isn't a straightforward package to review. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Readme file requirements
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.)
UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Functionality
Final approval (post-review)
Estimated hours spent reviewing: ~4h Review CommentsFirst of all, congratulations for the work done so far. The package is user-friendly, well-documented, and highly valuable for scientists. Moreover, the community is very reactive and helpful, especially on the chat. Here is a detailed review addressing some points that are mandatory in the review template and some other points/suggestions that could be useful for the future of the package. DocumentationI tried my best to review the whole documentation, but Installation instructionsInstalling
It would be nice to have a single page with all the installation instructions (including conda, pip, as well as virtual env creation in both cases). It may be too much for the README.md file, but you can have a simple version (for pip and conda) in the README.md file and a link to a more detailed version in the documentation. Not to mention that if you want to install A "Troubleshooting" section could be used to address common issues (e.g., Vignette(s)There is a code snippet in the README.md file; it runs fine if you install Community guidelinesThe contribution guide is well-documented. The "How to Contribute to sunpy" section is easy to find in the README and also appears in the documentation nav bar (but it's not the same link). The "Newcomers" section is very useful, but the reference to the MetadataAuthors are listed here; there is no individual email but a generic email (Google Group mail Badges
How the package compares to other similar packagesIs there somewhere in the documentation a comparison with other packages? I saw a page dedicated to affiliated packages but not a comparison with other packages. For example, how does Citation informationThe link in the readme file is not working properly; it should be here instead of here (maybe you should change the URL for something more meaningful than UsabilityThe package is really easy to use, past the installation step. Again, the documentation is well-written and easy to find. The need for the package is clear, and (almost) all functions have documentation and associated examples. Details on the installation testsI made some tests to install Pip:
Conda:
15 min later... finally ok I suppose it's due to the many dependencies of How to improve the installation process?: Sunpy is a large package with many dependencies (including astropy which also comes with many dependencies). Using optional dependencies in pip is a good idea. To help even more, you should see if splitting Sunpy into independent namespaces is feasible, to allow users to install only the submodule (and its dependencies) they need (and keep the FunctionalityTestsI tried to install a development version of python -m venv .venv
# activate, then
pip install -e ".[tests]"Then using to run the tests: pytestI got the following error. So I installed 20 failed, 2356 passed, 261 skipped, 1 xfailedWith 20 times the same error: Seems to be an issue with Nevertheless, the test coverage is very high, and the tests are well-written (I didn't manage to check all of them but randomly picked some of them). Quality and lintingPerhaps the part where But be careful, your Flake8 and pylint tools return several errors (line length, missing spaces, line breaks, variable name too short, snake case, I can only encourage you to use Also be careful to highlight good practices in the documentation. Indeed, using a single letter to name a variable is not recommended at all. Extract from the doc: There is no linting check in the CI to ensure that the code is compliant with the PEP 8 guidelines. TypingI also saw that a lot of attention had been paid to adding types in the documentation even though they are almost absent from the code. It could be a real plus to use Conclusion and suggestionsTo summarize, Now I will try to give some suggestions based on the feedback I received. It's a bit out of the scope of the review, but I think it could be useful for the future of the package. While Otherwise, some scientists develop recipes/code snippets to manipulate data that could be useful to the community. Is there any discussion to set up a section in the documentation (like the matplotlib gallery) to expose these recipes? Again, thank you for inviting me to review this package, and congratulations for the work done so far. I hope my review will help you to improve the package even more. Thanks to @somusset and @BaptisteCecconi for their feedback on the use of sunpy! |
|
Thank you @nutjob4life and @Septaris for your reviews!
@Septaris, this is fine for me too: you pointed out that the sunpy maintainers and the community around the package are quite responsive, so this not seems to be an issue. I have changed the status of the issue as "awaiting changes": I understand some comments can be addressed while others are more long term suggestions. |
|
Thank you @Septaris for reviewing the package!
On my todo list is to rewrite that page and the entire development section of the docs.
This is a good point and one we have struggled with. I think at this point, we will remove the install instructions from the readme and just link to the rendered documentation. There are definitely improvements to be made to those RST pages but I think at this point we consider using pip (for users) to be more advanced than using conda. As a result that is one reason that the pip information was put into the install topic guide. Whether this is the best way, time will tell.
I will remove this example and link to the tutorial and the example gallery instead.
Yes that has been a concern for a long time. The reason we link to the astropy one is to avoid creating another git/github guide when the astropy one exists and does a better job than we can do.
Our readme only links to the main branches CI, so will resolve to whatever GitHub ACtions wants to. Why it points to 5.0.1, I am surprised.
Good spot, I will update that to point to conda-forge.
We dropped 3.8 and in theory we should be able to support 3.12 but some of our upstream dependencies do not compile on 3.12, we have a draft PR keeping track of what is breaking. Hopefully soon we can add 3.12 support.
There is no page about how sunpy relates to astropy. I can open a tracking issue about that on our side.
Good point, I will fix that and update the url.
I will bring this topic up with the other maintainers, this seems like an interesting path forward.
Ah yeah, we in our testing documentation, mention this
Ah this is running the figure tests which need the
I will update it!
This is my plan but requires the consensus of the maintainers which has not happened.
This (alongside
We have a pre-commit check that runs the current config as our check, so if we get black there, that should account for this.
This is another topic that we need maintainer consensus on, it's pretty split over if we want to add typing support.
This is a excellent point and one I will bring up.
Thank you for spending so much time reviewing sunpy! |
Yes that will be enough. Thank you very much again. |
|
Hello everyone, I have opened this PR: sunpy/sunpy#7325 It changes the readme by deleting the install and example sections and just links out to our RTD pages that have that information. I feel like this might be against the spirit of the review and wanted to ask if doing this would be ok? It also changes a few other minor things like using |
|
Here are my thoughts and suggestions based on your comments/the merge request:
About the Installation Instructions:
And now, you still need to ensure compliance with the PEP 8 rules... Good luck! 😄 |
I didn't add it since unlike PyPi, it shows the latest version as the most recently uploaded, so it points to 5.0.2 and not 5.1. I don't want to give the impression that conda is outdated. Maybe I should ask upstream if they can change this.
I forgot to add that, how about instead I make sure that those tests are not run by default with pytest. Since we are comparing hashes, we pin versions of mpl and others that make a live environment less suited for these.
This is WIP, we have an open PR that we just need to fix some unit tests and we will have that.
I will say that our user base is Mac OS dominated and they removed system Python from that OS. Only linux distros ship with Python by default if I recall. But I do see your point and I will further adjust the text. I opened sunpy/sunpy#7343 to address the flake8 issues. |
|
Hello everybody, Thanks and happy Christmas and end of the year! |
|
Hello everyone, So we are still working on getting PRs reviewed and merged, ready for an upcoming release. I would suggest we need another week to get that done. Sorry for the delay on addressing the reviewers comments. |
No problem from the editor side... :) |
|
Hello everyone, Sorry again for the slowness from our side: The following PRs were merged in: sunpy/sunpy#7325 This should address ~85% of the review comments. Hopefully these will be good enough but I am happy to address any issues that I missed. I plan to follow up in a few weeks with a more comprehensive rewrite of the developer's guide to tie in with the installation guide suggestions from this review. You will also notice we added several flake8 ignore rules due to failing to achieve consensus over a full style and that will require a format that will allow us to not put spaces around specific operators that deal with astropy units. The readme table in the release version is actually a rst table where as the one in main is a raw::html, that will change in main with another PR (sunpy/sunpy#7384), it also updates the packaging of sunpy to use nothing but pyproject.toml. There is a different PR that needs another review which will raise a message if you import a submodule without the optional dependencies installed (sunpy/sunpy#7369) I plan to also submit a pr removing the These will be longer term PR goals. |
cmarmo
added
5/awaiting-reviewer-response
and removed
4/reviews-in-awaiting-changes
labels
|
Hello @nabobalis, thank you for the update! I think we can acknowledge the work done here: long term goals are always more challenging in complex projects. @nutjob4life , @Septaris , do you mind letting us know whether the maintainers answered to all your remarks or there are still some pending comments to be addressed? Thank you so much all for your work so far! |
|
@cmarmo good to go! |
|
Hello @nabobalis, You've done a great job 🎉, and indeed, you've addressed the main issues. Regarding the ignore rules, as long as you know what you're doing, it's fine. I hope you'll move towards a fully automated solution for code formatting to save you from having to manually format the code and/or add flake8 ignore rules. Good luck for the rewrite of the developer's guide! 😃 @cmarmo I think we're ready to conclude the review. |
|
Hello everybody, 🎉 it is my pleasure to announce that SunPy has been approved by pyOpenSci! Thank you @nabobalis for submitting SunPy and many thanks to @nutjob4life and @Septaris for reviewing this package! 😸 If you all could find some time to fill out our post-review survey this will help us a lot! Thanks! Also, let us know if you are interested in joining the pyOpenSci Slack (if you are not already there). Author Wrap Up TasksThere are a few things left to do to wrap up this submission:
Editor Final ChecksPlease 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. |
|
@nabobalis , when you and the SunPy community think it would be appropriate, please let us know how to communicate about this review (blog post, Discourse announcement, ....), thanks! |
|
Thank you again @cmarmo, @nutjob4life and @Septaris. I have added the badge to this PR: https://github.com/sunpy/sunpy/pull/7384/files?short_path=7b3ed02#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168f I put it under the community section in a new row. I think maybe a 4th column would be better as I also want to add a link to our joss paper and our latest published paper on sunpy. I have updated the release branches separately.
I will ask our social manager but I would be leaning towards adding a blogpost on sunpy.org and a discourse post on our openastro instance. Would pyopensci post an announcement on their own blog or discourse? |
The editor generally announce the successful review on discourse: I would be happy to cross-link any announcement on your side on that same occasion. |
|
That sounds good to me. I can also create a blog post for our own website. |
|
hey @nabobalis ! When you have a blog post on your website, please let us know! @kierisi can share it on our socials and we can also cross publish something similar and short on our blog as well linking to yours! In the meantime are you also ok if we post about pyOpenSci accepting sunpy on socials as well? that is fairly standard for us to welcome a new package into our ecosystem! |
Sounds good to me. |
|
I have written up sunpy/sunpy.org#401 I am hoping to get some reviews on this post this week so it can be merged. |
|
Thank you @nabobalis: without further ado let's declare sunPy accepted into the pyOpenSci ecosystem! Thank you to all people involved in the process! |
|
Thank you again @cmarmo, @nutjob4life and @Septaris for going through the review process and sticking with it as I made the changes! |
Submitting Author: Nabil Freij (@nabobalis)
All current maintainers: @ehsteve,@dpshelio,@wafels,@ayshih,@Cadair,@nabobalis,@dstansby,@DanRyanIrish,@wtbarnes,@ConorMacBride,@alasdairwilson,@hayesla,@vn-ki
Package Name: sunpy
One-Line Description of Package: Python for Solar Physics
Repository Link: https://github.com/sunpy/sunpy
Version submitted: 5.0.1
EiC: @isabelizimm
Editor: @cmarmo
Reviewer 1: @Septaris
Reviewer 2: @nutjob4life
Archive:
Version accepted: 5.1.1
JOSS DOI:
Date accepted (month/day/year): 01/18/2024
Code of Conduct & Commitment to Maintain Package
Description
Scope
Please indicate which category or categories.
Check out our package scope page to learn more about our
scope. (If you are unsure of which category you fit, we suggest you make a pre-submission inquiry):
Domain Specific
Community Partnerships
If your package is associated with a pyOpenSci partner community, please check below:
Technical checks
For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:
I will need to double check the examples, we have documentation for all public API
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
submission and improve our peer review process. We will also ask our reviewers
and editors to fill this out.
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.
Comments from Nabil
Hopefully everything is correct!
I did prune the JOSS and editions parts of the template, I hope that is ok since they are not relevant for this review.
The text was updated successfully, but these errors were encountered: