`sunpy` Review
nabobalis opened this issue ยท 35 comments
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
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
- I agree to abide by pyOpenSci's Code of Conduct during the review process and in maintaining my package after should it be accepted.
- I have read and will commit to package maintenance after the review as per the pyOpenSci Policies Guidelines.
Description
- sunpy is a community-developed, free and open-source solar data analysis environment for Python. It includes an interface for searching and downloading data from multiple data providers, data containers for image and time series data, commonly used solar coordinate frames and associated transformations, as well as other functionality needed for solar data analysis.
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):- Data retrieval
- Data extraction
- Data processing/munging
- Data deposition
- Data validation and testing
- Data visualization[^1]
- Workflow automation
- Citation management and bibliometrics
- Scientific software wrappers
- Database interoperability
Domain Specific
- Geospatial
- Education
Community Partnerships
If your package is associated with a pyOpenSci partner community, please check below:
- astropy
- sunpy
- Pangeo
- My package adheres to the Pangeo standards listed in the pyOpenSci peer review guidebook
Technical checks
For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:
- does not violate the Terms of Service of any service it interacts with.
- uses an OSI approved license.
- contains a README with instructions for installing the development version.
- includes documentation with examples for all functions.
I will need to double check the examples, we have documentation for all public API - contains a tutorial with examples of its essential functions and uses.
- has a test suite.
- has continuous integration setup, such as GitHub Actions CircleCI, and/or others.
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.
- Yes I am OK with reviewers submitting requested changes as issues to my repo. Reviewers will then link to the issues in their submitted review.
Confirm each of the following by checking the box.
- I have read the author guide.
- I expect to maintain this package for at least 2 years and can help find a replacement for the maintainer (team) if needed.
Please fill out our survey
- Last but not least please fill out our pre-review survey. This helps us track
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.
Thank you kindly for this submission, @nabobalis ๐ I also appreciate your preparedness beforehand going through our checklists!
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.
- Installation The package can be installed from a community repository such as PyPI (preferred), and/or a community channel on conda (e.g. conda-forge, bioconda).
- The package imports properly into a standard Python environment
import package
.
- The package imports properly into a standard Python environment
- Fit The package meets criteria for fit and overlap.
- Documentation The package has sufficient online documentation to allow us to evaluate package function and scope without installing the package. This includes:
- User-facing documentation that overviews how to install and start using the package.
- Short tutorials that help a user understand how to use the package and what it can do for them.
- API documentation (documentation for your code's functions, classes, methods and attributes): this includes clearly written docstrings with variables defined using a standard docstring format.
- Core GitHub repository Files
- README The package has a
README.md
file with clear explanation of what the package does, instructions on how to install it, and a link to development instructions. - Contributing File The package has a
CONTRIBUTING.md
file that details how to install and contribute to the package. - Code of Conduct The package has a
CODE_OF_CONDUCT.md
file. - License The package has an OSI approved license.
NOTE: We prefer that you have development instructions in your documentation too.
- README The package has a
- Issue Submission Documentation All of the information is filled out in the
YAML
header of the issue (located at the top of the issue template). - Automated tests Package has a testing suite and is tested via a Continuous Integration service.
- Repository The repository link resolves correctly.
- Package overlap The package doesn't entirely overlap with the functionality of other packages that have already been submitted to pyOpenSci.
- Archive (JOSS only, may be post-review): The repository DOI resolves correctly.
- Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?
- Initial onboarding survey was filled out
We appreciate each maintainer of the package filling out this survey individually. ๐
Thank you authors in advance for setting aside five to ten minutes to do this. It truly helps our organization. ๐
Editor comments
The 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, sunpy
has CONTRIBUTING and CODE_OF_CONDUCT files at the org level. Since sunpy is not only a package, but also a community, this is a good setup for them to have the same standards across all their packages. For this reason, it is not necessary for these files to live in the root directory of the sunpy/sunpy repository that is under review.
Your editor for this review will be @cmarmo
Thank you @isabelizimm for pre-reviewing the submission.
Editor comments
The 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?
I will list the names in the original post as the team has not changed in a while.
I am surprised that it isn't public, I will see if I can get that fixed.
Hi @nabobalis, nice to meet you, and thanks for submitting to pyOpenSci!
I'm Chiara and I'm going to take care of the process as editor.
I'm looking for reviewers right now and hope to be back to you by the end of the next week.
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.
I hope to have some answers next week.
Thanks for your patience!
Hi @nabobalis,
thank you for your patience!
Let's welcome here @Septaris and @nutjob4life who kindly volunteered to review SunPy for pyOpenSci! ๐
Thank you both! Please take some time to introduce yourself here before starting the review.
Please fill out our pre-review survey
Before 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.
- Sonny Lion survey completed.
- Sean Kelly survey completed.
The following resources will help you complete your review:
- Here is the reviewers guide. This guide contains all of the steps and information needed to complete your review.
- Here is the review template that you will need to fill out and submit here as a comment, once your review is complete.
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 sunpy
and contribute to the open-source community. I'll get started as soon as I have a moment ๐
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 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
- As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).
Documentation
The package includes all the following forms of documentation:
- A statement of need clearly stating problems the software is designed to solve and its target audience in README.
- Installation instructions: for the development version of the package and any non-standard dependencies in README.
- Vignette(s) demonstrating major functionality that runs successfully locally.
- Function Documentation: for all user-facing functions.
- Examples for all user-facing functions.
- Community guidelines including contribution guidelines in the README or CONTRIBUTING.
- Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a
pyproject.toml
file or elsewhere.
Readme file requirements
The package meets the readme requirements below:
- Package has a README.md file in the root directory.
The README should include, from top to bottom:
- The package name
- Badges for:
- Continuous integration and test coverage,
- Docs building (if you have a documentation website),
- A repostatus.org badge,
- Python versions supported,
- Current package version (on PyPI / Conda).
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.)
- Short description of package goals.
- Package installation instructions
- Any additional setup required to use the package (authentication tokens, etc.)
- Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
- Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
- Link to your documentation website.
- If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
- Citation information
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:
- Package documentation is clear and easy to find and use.
- The need for the package is clear
- All functions have documentation and associated examples for use
- The package is easy to install
Functionality
- Installation: Installation succeeds as documented.
- Functionality: Any functional claims of the software been confirmed.
- Performance: Any performance claims of the software been confirmed.
- Automated tests:
- All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
- Tests cover essential functions of the package and a reasonable range of inputs and conditions.
- Continuous Integration: Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
- Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.
A few notable highlights to look at:- Package supports modern versions of Python and not End of life versions.
- Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)
For packages also submitting to JOSS
- The package has an obvious research application according to JOSS's definition in their submission requirements.
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:
- A short summary describing the high-level functionality of the software
- Authors: A list of authors with their affiliations
- A statement of need clearly stating problems the software is designed to solve and its target audience.
- References: With DOIs for all those that have one (e.g. papers, datasets, software).
Final approval (post-review)
- The author has responded to my review and made changes to my satisfaction. I recommend approving this package.
Estimated hours spent reviewing: 1.25 hours
Review Comments
First 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:
- The "vignette" on the package's homepage
README.md
works only if you're using Conda. It should work with a bare Python:
$ 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 README.md
could mention that the [map]
extra is required for the vignette? This also applies to the "Any additional setup required to use the package" criterion.
-
The API is enormous, so I can only spot-check if there's "Function Documentation: for all user-facing functions". But looks good.
-
The same applies to "Examples for all user-facing functions", however I find that
sunpy.util.expand_list
does not have an example. (This also applies to the "All functions have documentation and associated examples for use" criterion.) -
Python packaging has been (sadly) a moving target, but this project seems to find the balance between
setup.cfg
andpyproject.toml
. (The Python Packaging Authority has finally settled on moving away fromsetup.py
andsetup.cfg
so perhaps later versions ofsunpy
can move all project metadata topyproject.toml
in a future version.) -
"Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file". There is indeed one vignette in the
README.md
however I would not call this a small package! ๐ -
"Any performance claims of the software been confirmed" โฆ the documentation doesn't make any mention of performance, so this criterion easily passes ๐.
-
Running
flake8 --count sunpy
for "Code format is standard throughout package and follows PEP 8 guidelines" shows 501 violations -
JOSS criteria: version 1.0.8 was reviewed by JOSS, but this version (5.0.1) does not have
paper.md
. Is version 5.0.1 going to be submitted to JOSS? If so, we'll need this file. If not, then no worries ๐
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.
Review Comments
First 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:
- The "vignette" on the package's homepage
README.md
works only if you're using Conda. It should work with a bare Python:$ 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
README.md
could mention that the[map]
extra is required for the vignette? This also applies to the "Any additional setup required to use the package" criterion.
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 map
extra.
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.
- The API is enormous, so I can only spot-check if there's "Function Documentation: for all user-facing functions". But looks good.
Yeah, I tried to go through it myself and got lost midway.
- The same applies to "Examples for all user-facing functions", however I find that
sunpy.util.expand_list
does not have an example. (This also applies to the "All functions have documentation and associated examples for use" criterion.)
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.
- Python packaging has been (sadly) a moving target, but this project seems to find the balance between
setup.cfg
andpyproject.toml
. (The Python Packaging Authority has finally settled on moving away fromsetup.py
andsetup.cfg
so perhaps later versions ofsunpy
can move all project metadata topyproject.toml
in a future version.)
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.
- "Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file". There is indeed one vignette in the
README.md
however I would not call this a small package! ๐
While we do link to our example gallery in the readme, it's not very obvious.
I think there is much better formatting and layout work to be done to the readme to make it much better.
- "Any performance claims of the software been confirmed" โฆ the documentation doesn't make any mention of performance, so this criterion easily passes ๐.
Luckily we don't ๐
- Running
flake8 --count sunpy
for "Code format is standard throughout package and follows PEP 8 guidelines" shows 501 violations
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.
- JOSS criteria: version 1.0.8 was reviewed by JOSS, but this version (5.0.1) does not have
paper.md
. Is version 5.0.1 going to be submitted to JOSS? If so, we'll need this file. If not, then no worries ๐
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.
That's all I got! Thanks for the opportunity to review this.
Thank you for going through the package, sunpy has really grown and it isn't a straightforward package to review.
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
- As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).
Documentation
The package includes all the following forms of documentation:
- A statement of need clearly stating problems the software is designed to solve and its target audience in README.
- Installation instructions: for the development version of the package and any non-standard dependencies in README.
- Vignette(s) demonstrating major functionality that runs successfully locally.
- Function Documentation: for all user-facing functions.
- Examples for all user-facing functions.
- Community guidelines including contribution guidelines in the README or CONTRIBUTING.
- Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a
pyproject.toml
file or elsewhere.
Readme file requirements
The package meets the readme requirements below:
- Package has a README.md file in the root directory.
The README should include, from top to bottom:
- The package name
- Badges for:
- Continuous integration and test coverage,
- Docs building (if you have a documentation website),
- A repostatus.org badge,
- Python versions supported,
- Current package version (on PyPI / Conda).
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.)
- Short description of package goals.
- Package installation instructions
- Any additional setup required to use the package (authentication tokens, etc.)
- Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
- Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
- Link to your documentation website.
- If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
- Citation information
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:
- Package documentation is clear and easy to find and use.
- The need for the package is clear
- All functions have documentation and associated examples for use
- The package is easy to install
Functionality
- Installation: Installation succeeds as documented.
- Functionality: Any functional claims of the software been confirmed.
- Performance: Any performance claims of the software been confirmed.
- Automated tests:
- All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
- Tests cover essential functions of the package and a reasonable range of inputs and conditions.
- Continuous Integration: Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
- Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.
A few notable highlights to look at:- Package supports modern versions of Python and not End of life versions.
- Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)
Final approval (post-review)
- The author has responded to my review and made changes to my satisfaction. I recommend approving this package.
Estimated hours spent reviewing: ~4h
Review Comments
First 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.
Documentation
I tried my best to review the whole documentation, but sunpy
is a substantial package. Some user-facing functions may not be documented, and some examples missing, but most of them are well-documented. I will focus on a few points that could, in my opinion, be improved.
Installation instructions
Installing sunpy
is not that easy, and this is not due to sunpy
but to its many dependencies. The installation process is well-documented but instructions are disseminated in multiple places:
- For
conda
: README.md and Installation Guide - For both
pip
andconda
: Topic Guide - For the development version: Conda for Dependencies
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 sunpy
with pip
, you have to specify flags to install the dependencies (e.g., pip install sunpy[all]
), which is a very good practice but can be confusing for beginners, so it would be nice to mention it as well in the README.md file.
A "Troubleshooting" section could be used to address common issues (e.g., no module named astropy
).
Vignette(s)
There is a code snippet in the README.md file; it runs fine if you install sunpy[all]
. Maybe you could include the generated plot in the README.md file to show the result? Plus, a link to the tutorial to go further.
Community guidelines
The 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 astropy
guide can be confusing for newcomers.
Metadata
Authors are listed here; there is no individual email but a generic email (Google Group mail sunpy@googlegroups.com
, plus the chat link). For me, it's fine. To be discussed with @cmarmo.
Badges
- Continuous integration: CI is successful for
5.0.1
here, but not for recent versions nor for5.0.2
. - Current package version (on PyPI / Conda): PyPI badge is here, but not the conda badge here while conda is recommended in the documentation.
- Python versions supported: 3.8 and 3.12 are missing in the badge; are they supported?
How the package compares to other similar packages
Is 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 sunpy
relate to astropy
?
Citation information
The 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 about
)
Usability
The 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 tests
I made some tests to install sunpy
with pip
and conda
, and here are the results (tests performed with Python 3.11 (Windows 10), 3.10 (Ubuntu) and Conda 23.10 (Windows 10)):
Pip:
pip install sunpy==5.0.1
: ok (as well as with flags likeall
,tests
, etc.)pip install .[dev]
: okpip install -e .[dev]
: ok
Conda:
conda install sunpy==5.0.1
:
Collecting package metadata (current_repodata.json): done
Solving environment: failed with initial frozen solve. Retrying with flexible solve.
Solving environment: failed with repodata from current_repodata.json, will retry with next repodata source.
Collecting package metadata (repodata.json): done
Solving environment: -
15 min later... finally ok
I suppose it's due to the many dependencies of sunpy
; I used the --debug
flag to see what was going on and be sure there was no crash during the 15 min...
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 all
mechanism for the full installation). But I know it's a huge work... Further on I mention the need to regularly add new clients/instruments/missions, and this could also be done with a plugin system.
Functionality
Tests
I tried to install a development version of sunpy
and run the tests in a fresh environment:
python -m venv .venv
# activate, then
pip install -e ".[tests]"
Then using pytest
to run the tests:
pytest
I got the following error.
ModuleNotFoundError: No module named 'scipy'
So I installed sunpy[all]
to be sure to have all the dependencies and run the tests again. This time most of the tests passed, but some tests failed on my computer. Here is a snippet of the results:
20 failed, 2356 passed, 261 skipped, 1 xfailed
With 20 times the same error:
FAILED sunpy/map/tests/test_compositemap.py::test_peek_composite_map - UserWarning: FigureCanvasAgg is non-interactive, and thus cannot be shown
Seems to be an issue with matplotlib
.
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 linting
Perhaps the part where sunpy
could be improved the most. There is already some useful tools set up like pre-commit
with ruff
and isort
:
ruff.....................................................................Passed
isort....................................................................Passed
check python ast.........................................................Passed
check for case conflicts.................................................Passed
trim trailing whitespace.................................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
check for added large files..............................................Passed
fix end of files.........................................................Passed
mixed line ending........................................................Passed
generate sunpy.net.hek.attrs.............................................Passed
git diff.................................................................Passed
codespell................................................................Passed
But be careful, your ruff
version is outdated!
Flake8 and pylint tools return several errors (line length, missing spaces, line breaks, variable name too short, snake case, import *
with no justification).
I can only encourage you to use black
which will already drastically reduce the number of errors and will allow you to comply very quickly with PEP 8. Then go over the remaining errors to mark them as acceptable or not via noqa
and pylint:disable
.
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:
# https://docs.sunpy.org/en/stable/tutorial/acquiring_data/index.html#acquiring-data
from sunpy.net import Fido, attrs as a
There is no linting check in the CI to ensure that the code is compliant with the PEP 8 guidelines.
Typing
I 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 mypy
to do static type checking.
Conclusion and suggestions
To summarize, sunpy
provides a really nice and stable interface to download data from many missions and instruments, easily plot and manipulate the data, and handle units and coordinates changes seamlessly. Everything is accessible in a single package! The Python package is well structured and documented, and easy to use (as soon as it is installed!). The community is very active and helpful. The installation process is a bit complicated, but it's not due to sunpy
itself but to its many dependencies. The tests are well-written, and the test coverage is very high. The quality of the code has to be improved to conform to the PEP 8 guidelines, and black
could be used to help with that.
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 sunpy
is already very complete, it is still missing a lot of missions/instruments/databases, for instance for radio and X-ray data. More generally, there is no clear recipe on how to add a dataset to sunpy. There is indeed the beginning of an explanation with the example around GenericMap
here, but it's not enough for beginners. It would be nice to have a clear recipe on how to add a new dataset to sunpy. To push one step further the concept of affiliated packages like radiospectra, you can imagine a plugin system to handle new datasets.
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!
Sunpy is a mature package and reviewing it is a lot of work.
Authors are listed here; there is no individual email but a generic email (Google Group mail sunpy@googlegroups.com, plus the chat link). For me, it's fine.
@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.
Also, in order to allow the maintainers to reach a consensus about the modifications, @nabobalis, do you think a three weeks interval would be enough?
Thank you for your collaboration!
Thank you @Septaris for reviewing the package!
Installation instructions
Installing
sunpy
is not that easy, and this is not due tosunpy
but to its many dependencies. The installation process is well-documented but instructions are disseminated in multiple places:
- For
conda
: README.md and Installation Guide- For both
pip
andconda
: Topic Guide- For the development version: Conda for Dependencies
On my todo list is to rewrite that page and the entire development section of the docs.
But yeah, installing sunpy is partly a problem about making sure users install a python distribution that is seperate from their system and ideally not anaconda either.
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
sunpy
withpip
, you have to specify flags to install the dependencies (e.g.,pip install sunpy[all]
), which is a very good practice but can be confusing for beginners, so it would be nice to mention it as well in the README.md file.
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.
Vignette(s)
There is a code snippet in the README.md file; it runs fine if you install
sunpy[all]
. Maybe you could include the generated plot in the README.md file to show the result? Plus, a link to the tutorial to go further.
I will remove this example and link to the tutorial and the example gallery instead.
Community guidelines
The 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
astropy
guide can be confusing for newcomers.
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.
Badges
- Continuous integration: CI is successful for
5.0.1
here, but not for recent versions nor for5.0.2
.
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.
- Current package version (on PyPI / Conda): PyPI badge is here, but not the conda badge here while conda is recommended in the documentation.
Good spot, I will update that to point to conda-forge.
- Python versions supported: 3.8 and 3.12 are missing in the badge; are they supported?
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.
How the package compares to other similar packages
Is 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
sunpy
relate toastropy
?
There is no page about how sunpy relates to astropy. I can open a tracking issue about that on our side.
Citation information
The 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
about
)
Good point, I will fix that and update the url.
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
all
mechanism for the full installation). But I know it's a huge work... Further on I mention the need to regularly add new clients/instruments/missions, and this could also be done with a plugin system.
I will bring this topic up with the other maintainers, this seems like an interesting path forward.
Functionality
Tests
I tried to install a development version of
sunpy
and run the tests in a fresh environment:python -m venv .venv # activate, then pip install -e ".[tests]"Then using
pytest
to run the tests:
pytestI got the following error.
ModuleNotFoundError: No module named 'scipy'
So I installed
sunpy[all]
to be sure to have all the dependencies and run the tests again.
Ah yeah, we in our testing documentation, mention this ["all,tests"]
.
This is one of the many small pain points of running our test suite.
This time most of the tests passed, but some tests failed on my computer. Here is a snippet of the results:
20 failed, 2356 passed, 261 skipped, 1 xfailedWith 20 times the same error:
FAILED sunpy/map/tests/test_compositemap.py::test_peek_composite_map - UserWarning: FigureCanvasAgg is non-interactive, and thus cannot be shown
Seems to be an issue with
matplotlib
.
Ah this is running the figure tests which need the MPLBACKEND = agg
to be set. This is done automatically if you run the tests via tox but not if its done via pytest.
Quality and linting
But be careful, your
ruff
version is outdated!
I will update it!
Flake8 and pylint tools return several errors (line length, missing spaces, line breaks, variable name too short, snake case,
import *
with no justification).I can only encourage you to use
black
which will already drastically reduce the number of errors and will allow you to comply very quickly with PEP 8. Then go over the remaining errors to mark them as acceptable or not vianoqa
andpylint:disable
.
This is my plan but requires the consensus of the maintainers which has not happened.
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:
# https://docs.sunpy.org/en/stable/tutorial/acquiring_data/index.html#acquiring-data from sunpy.net import Fido, attrs as a
This (alongside import astropy.units as u
) will not change as I think at this point, we are too far down the legacy route. But I can bring this up with the other maintainers to see if we can get this changed.
There is no linting check in the CI to ensure that the code is compliant with the PEP 8 guidelines.
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.
Typing
I 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
mypy
to do static type checking.
This is another topic that we need maintainer consensus on, it's pretty split over if we want to add typing support.
Conclusion and suggestions
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
sunpy
is already very complete, it is still missing a lot of missions/instruments/databases, for instance for radio and X-ray data. More generally, there is no clear recipe on how to add a dataset to sunpy. There is indeed the beginning of an explanation with the example aroundGenericMap
here, but it's not enough for beginners. It would be nice to have a clear recipe on how to add a new dataset to sunpy. To push one step further the concept of affiliated packages like radiospectra, you can imagine a plugin system to handle new datasets.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?
This is a excellent point and one I will bring up.
We have not done enough outreach to help scientists contribute back to sunpy and starting with better examples and trying to get a more encompassing gallery (aka learn.astropy.org) is on the planning list.
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.
Thank you for spending so much time reviewing sunpy!
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. Also, in order to allow the maintainers to reach a consensus about the modifications, @nabobalis, do you think a three weeks interval would be enough? Thank you for your collaboration!
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 [test]
will also pull in all
among other changes that the reviewers spotted.
Here are my thoughts and suggestions based on your comments/the merge request:
- Conda Badge: The Conda badge is still missing.
- Continuous Integration: If the current behavior regarding continuous integration is intentional, then it's fine.
- Citation Link: The link has been renamed/updated. โ
- Tests Documentation: Regarding the Matplotlib backend (MPLBACKEND = agg), it would be nice to include this information in the pytest-related documentation.
- Package supports modern versions of Python: Support for version 3.8 will be dropped in a few months, so it's not a problem. However, support for version 3.12 seems required.
About the Installation Instructions:
-
The review grid is strict and indicates the need to integrate installation instructions for the development version and vignette(s) directly into the README. However, from my perspective, it's acceptable if you propose a more streamlined approach by providing links to the gallery and standard/development installation instructions in the README. It seems reasonable and can prevent information duplication. I'll let our editor, @cmarmo, decide.
-
Now, on the installation documentation content, specifically
pip
vsconda
: From my experience, installingsunpy
is easier via pip (sunpy[all]
) than via conda, especially on a standard operating system where thewheels
are available. I'm afraid that beginners who usepip
(which remains the Python standard) might get confused at the first mention ofconda
. Therefore, I recommend the following structure:- A standard installation procedure (with Conda and Pip sections).
- An installation procedure in development mode (again with Conda and Pip sections).
- A readme that points to the two installation procedures.
Note: this is only a suggestion and is not a blocking point for the review. I will let you decide based on the needs/feedback of the community.
And now, you still need to ensure compliance with the PEP 8 rules... Good luck! ๐
- Conda Badge: The Conda badge is still missing.
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.
- Tests Documentation: Regarding the Matplotlib backend (MPLBACKEND = agg), it would be nice to include this information in the pytest-related documentation.
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.
- Package supports modern versions of Python: Support for version 3.8 will be dropped in a few months, so it's not a problem. However, support for version 3.12 seems required.
This is WIP, we have an open PR that we just need to fix some unit tests and we will have that.
About the Installation Instructions:
Now, on the installation documentation content, specifically
pip
vsconda
: From my experience, installingsunpy
is easier via pip (sunpy[all]
) than via conda, especially on a standard operating system where thewheels
are available. I'm afraid that beginners who usepip
(which remains the Python standard) might get confused at the first mention ofconda
. Therefore, I recommend the following structure:
- A standard installation procedure (with Conda and Pip sections).
- An installation procedure in development mode (again with Conda and Pip sections).
- A readme that points to the two installation procedures.
Note: this is only a suggestion and is not a blocking point for the review. I will let you decide based on the needs/feedback of the community.
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.
This means they need to install Python and when it also comes to arm on Mac, for me Conda makes supporting that easier.
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,
touching base as the three weeks have passed and I didn't realize the deadline was bringing us just before Christmas holidays! ๐
To sum up: the pull requests addressing the reviewer comments are sunpy/sunpy#7325 and sunpy/sunpy#7343, which still need approval from the SunPy community.
I'd like to make the vacation break official: I'm going to check in around January 10th so we can finalize the discussion.
Let me know if this is ok for all the people involved.
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.
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... :)
Thanks for letting us know.
Hello everyone,
Sorry again for the slowness from our side:
The following PRs were merged in:
sunpy/sunpy#7325
sunpy/sunpy#7351
sunpy/sunpy#7343
This should address ~85% of the review comments.
These form the new releases 5.0.3 and 5.1.1 which are out on pypi and conda-forge.
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 import attrs as a
style we have been using.
These will be longer term PR goals.
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 Tasks
There are a few things left to do to wrap up this submission:
- Activate Zenodo watching the repo if you haven't already done so.
- Tag and create a release to create a Zenodo version and DOI.
- Add the badge for pyOpenSci peer-review to the README.md of SunPy. The badge should be
[![pyOpenSci](https://tinyurl.com/y22nb8up)](https://github.com/pyOpenSci/software-review/issues/issue-number)
. .... sorry the table would become even more challenging... - Please fill out the post-review survey. All maintainers and reviewers should fill this out.
Editor Final Checks
Please complete the final steps to wrap up this review. Editor, please do the following:
- Make sure that the maintainers filled out the post-review survey
- Invite the maintainers to submit a blog post highlighting their package. Feel free to use / adapt language found in this comment to help guide the author.
- Change the status tag of the issue to
6/pyOS-approved6 ๐๐๐
. - Invite the package maintainer(s) and both reviewers to slack if they wish to join.
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.
@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!
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?
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!
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!
I'm going to close this issue and move the announcement on the pyOpenSci discourse.
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!