Submit `solarmach` as an affiliated package

jgieseler commented 11 months ago

Package Details

Package name: solarmach
Maintainer(s): Jan Gieseler
Repository: https://github.com/jgieseler/solarmach
Documentation link: https://solarmach.readthedocs.io
PyPI link (if applicable): https://pypi.org/project/solarmach
Continuous integration link(s): https://github.com/jgieseler/solarmach/blob/main/.github/workflows/pytest.yml
Link to Code of Conduct or page describing adopted Code of Conduct: https://github.com/jgieseler/solarmach/blob/main/code_of_conduct.md

Description of Package

The Solar MAgnetic Connection HAUS tool (solarmach) is a Python package that derives (using sunpy) and visualizes the spatial configuration and solar magnetic connection of different observers in the heliosphere at different times.

(Note: solarmach is the PyPI/conda-forge Python package that provides all functionalities to end users and to the streamlit web-app at https://solar-mach.github.io. The streamlit app itself is also open-source (cf. https://github.com/jgieseler/Solar-MACH), but not provided as a Python package.)

Package Review

Editor Submission Checklist

[x] Is the submission appropriate (compatible with the SunPy CoC)?
[x] Does the project have an appropriate license?
[x] Is the project in an online version control system?
[x] Does the project provide a Python interface?
[x] Is the project on PyPI?
[x] Is the project useful to the solar physics community?
[x] Version that was reviewed (tag or commit hash) - v0.2.2

Instructions to Reviewer

Please copy the following and select the ranking for each criteria, the full review criteria can be found here:

* Functionality           : General Package / Specialized Package / Not Relevant
* Integration             : Well integrated / Partially Integrated / Minimal Integration
* Documentation           : Extensive / Some / Little
* Testing                 : Excellent / Good / Needs Work
* Duplication             : None / Some / Major
* Community               : Excellent / Good / Needs Work
* Development Status      : Stable / Subject to Change / Low Activity / Needs Work

Cadair commented 11 months ago

:partying_face: Thanks for the submission @jgieseler I shall try and find you a reviewer next week.

jgieseler commented 11 months ago

I would suggest @wtbarnes, as he was a reviewer for our corresponding publication.

Cadair commented 11 months ago

@wtbarnes you up for reviewing this?

wtbarnes commented 7 months ago

Hi @jgieseler sorry for neglecting this for so long! I've not forgotten about this submission and am aiming to complete this review before Christmas. Sorry again this has been open for so long and thanks for your patience.

jgieseler commented 7 months ago

No worries @wtbarnes! There is no hurry from my side.

wtbarnes commented 3 months ago

First off, let me apologize profusely to @jgieseler for how prolonged this process has been. We definitely do not intend for these reviews to be open for this long and we greatly appreciate your patience. This is entirely my fault. See below for my full review.

I reviewed version v0.3.3 of the package, the latest released version

Functionality

Though this package is relatively small in scope, I'm classifying it as general because knowing the relative locations of observatories has broad applications across solar physics and heliophysics.

Integration

This package makes extensive use of astropy.coordinates and the appropriate solar coordinate frames provided by sunpy.coordinates. However, this package does not make use of astropy.units anywhere. This would be particularly helpful when it comes to specifying the solar wind speed. It may also be useful internally when ensuring that quantities are being plotted in compatible units. I see that jgieseler/solarmach#26 has already been created to track this. Additionally, I see that while sunpy.time.parse_time is used in the sc_distance function, it is not used elsewhere. I recommend using parse_time to check the time format as soon as the SolarMach class is instantiated. I've created jgieseler/solarmach#49 to track this.

Documentation

The hosted documentation provides both reference documentation as well as narrative documentation about how to use the documentation. I see that the SolarMach.plot_3d and SolarMach.pfss_3d methods are not included in the API reference. I see that jgieseler/solarmach#45 has already been created to track this.

This is not a requirement for affiliated package status, but I would also suggest separating out the "Usage" section into a "Tutorial" and "How-to Guide" sections. With a small amount of reorganizing, steps 1 through 4 could be a self-contained tutorial and the items under "Advanced" could be made into how-to guides.

Testing

There are unit tests for most functions and methods. I am marking this as "good" rather than "excellent" primarily due to the test coverage being at 65% according to the CodeCov badge. As the primary purpose of solarmach is producing figures, I would also highly suggest making using of figure tests. Currently, the results of the plotting tests are only validated by checking for the existence of the plot, e.g. https://github.com/jgieseler/solarmach/blob/9a6ab68e047a43b529c94ff4f73ec10697836654/solarmach/tests/test.py#L81. This provides no guarantee as to the correctness of this plot. I've created jgieseler/solarmach#50 to track this.

Duplication

There is no obvious duplication of any functionality currently available in the ecosystem.

Community

My only feedback here would be to provide some documentation on how to contribute to solarmach.

Development Status

I labeled this "subject to change" because the version number is <1

Decision

Per our review criteria, solarmach is now accepted as a SunPy affiliated package 🎉 . Congratulations and apologies this was so drawn out. I think the issues referenced in this review would still be helpful to address and would help integrate solarmach into the ecosystem even more.

jgieseler commented 3 months ago

Thanks a lot @wtbarnes for your review and especially the feedback, very much appreciated! I fully agree with all points mentioned, and already implemented the figure unit tests as this was something I was already unsatisfied with.

What about updates to the different categories if the project has evolved after some time? Should I at one point create a new pull request indicating what has been improved, or will this just be part of the yearly review process mentioned at https://sunpy.org/affiliated#existing-packages-review-process? Both would be fine with me.

wtbarnes commented 3 months ago

Thanks very much for submitting solarmach and apologies again this was held up for so long.

What about updates to the different categories if the project has evolved after some time?

That's a great question. In theory, yes this would be part of the yearly re-review process for all of the affiliated packages. In practice, we've never actually carried out a re-review of an affiliated package. This whole process also may change as we integrate our affiliated package review process into PyOpenSci.

sunpy / sunpy.org