search

pyOpenSci / software-submission

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

`sunpy` Review #147

Closed nabobalis closed 4 months ago

nabobalis commented 7 months ago

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: DOI Version accepted: 5.1.1 JOSS DOI: DOI Date accepted (month/day/year): 01/18/2024

Code of Conduct & Commitment to Maintain Package

Description

Scope

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:

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

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

Confirm each of the following by checking the box.

Please fill out our survey

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

Editor and Review Templates

The editor template can be found here.

The review template can be found here.

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.

isabelizimm commented 7 months ago

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.

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

nabobalis commented 7 months ago

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.

cmarmo commented 6 months ago

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.

nabobalis commented 6 months ago

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!

cmarmo commented 6 months ago

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!

cmarmo commented 6 months ago

Hi @nabobalis,

thank you for your patience!

Let's welcome here @Septaris and @nutjob4life who kindly volunteered to review SunPy for pyOpenSci! :wave: 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.

The following resources will help you complete your review:

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

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! :pray:

nabobalis commented 6 months ago

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!

Septaris commented 6 months ago

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 :smile:

nutjob4life commented 6 months ago

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.

nutjob4life commented 6 months ago

Package Review

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

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

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

Usability

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

Functionality

For packages also submitting to JOSS

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

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

Final approval (post-review)

Estimated hours spent reviewing: 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:

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

  2. The API is enormous, so I can only spot-check if there's "Function Documentation: for all user-facing functions". But looks good.

  3. 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.)

  4. Python packaging has been (sadly) a moving target, but this project seems to find the balance between setup.cfg and pyproject.toml. (The Python Packaging Authority has finally settled on moving away from setup.py and setup.cfg so perhaps later versions of sunpy can move all project metadata to pyproject.toml in a future version.)

  5. "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! 😆

  6. "Any performance claims of the software been confirmed" … the documentation doesn't make any mention of performance, so this criterion easily passes 😇.

  7. Running flake8 --count sunpy for "Code format is standard throughout package and follows PEP 8 guidelines" shows 501 violations

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

nabobalis commented 6 months ago

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:

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

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

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

  1. Python packaging has been (sadly) a moving target, but this project seems to find the balance between setup.cfg and pyproject.toml. (The Python Packaging Authority has finally settled on moving away from setup.py and setup.cfg so perhaps later versions of sunpy can move all project metadata to pyproject.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.

  1. "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.

  1. "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 😆

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

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

Septaris commented 6 months ago

Package Review

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

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

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

Usability

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

Functionality

Final approval (post-review)

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:

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

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:

Conda:

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 noqaand 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!

cmarmo commented 6 months ago

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!

nabobalis commented 5 months ago

Thank you @Septaris for reviewing the package!

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:

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

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 for 5.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 to astropy?

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:

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.

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

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 via noqaand pylint: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 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?

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!

nabobalis commented 5 months ago

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.

nabobalis commented 5 months ago

Hello everyone, I have opened this PR: https://github.com/sunpy/sunpy/pull/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.

Septaris commented 5 months ago

Here are my thoughts and suggestions based on your comments/the merge request:

About the Installation Instructions:

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

  2. Now, on the installation documentation content, specifically pip vs conda: From my experience, installing sunpy is easier via pip (sunpy[all]) than via conda, especially on a standard operating system where the wheels are available. I'm afraid that beginners who use pip (which remains the Python standard) might get confused at the first mention of conda. 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! 😄

nabobalis commented 5 months ago
  • 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:

  1. Now, on the installation documentation content, specifically pip vs conda: From my experience, installing sunpy is easier via pip (sunpy[all]) than via conda, especially on a standard operating system where the wheels are available. I'm afraid that beginners who use pip (which remains the Python standard) might get confused at the first mention of conda. 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 https://github.com/sunpy/sunpy/pull/7343 to address the flake8 issues.

cmarmo commented 5 months ago

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 https://github.com/sunpy/sunpy/pull/7325 and https://github.com/sunpy/sunpy/pull/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!

nabobalis commented 4 months ago

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.

cmarmo commented 4 months ago

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.

nabobalis commented 4 months ago

Hello everyone,

Sorry again for the slowness from our side:

The following PRs were merged in:

https://github.com/sunpy/sunpy/pull/7325 https://github.com/sunpy/sunpy/pull/7351 https://github.com/sunpy/sunpy/pull/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 (https://github.com/sunpy/sunpy/pull/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 (https://github.com/sunpy/sunpy/pull/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.

cmarmo commented 4 months ago

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!

nutjob4life commented 4 months ago

@cmarmo good to go!

Septaris commented 4 months ago

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.

cmarmo commented 4 months ago

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:

Editor Final Checks

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

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

cmarmo commented 4 months ago

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

nabobalis commented 4 months ago

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?

cmarmo commented 4 months ago

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.

nabobalis commented 4 months ago

That sounds good to me. I can also create a blog post for our own website.

lwasser commented 4 months ago

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!

nabobalis commented 4 months ago

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.

nabobalis commented 4 months ago

I have written up https://github.com/sunpy/sunpy.org/pull/401 I am hoping to get some reviews on this post this week so it can be merged.

cmarmo commented 4 months ago

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!

nabobalis commented 4 months ago

Thank you again @cmarmo, @nutjob4life and @Septaris for going through the review process and sticking with it as I made the changes!