Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal to add ZodiPy to affiliated packages (pyOpenSci/software-submission#161) #495

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

MetinSa
Copy link
Contributor

@MetinSa MetinSa commented Jul 2, 2022

EDIT: Also submitted pyOpenSci/software-submission#161

ZodiPy is a new Python tool for modelling the zodiacal emission seen by an arbitrary Solar System observer, which can be used for removal of both thermal emission and scattered sunlight from interplanetary dust in astrophysical data. One of our main goals with ZodiPy is to make zodiacal emission simulations more accessible by providing the community with a simple Python interface to existing models. We recently published a paper on ZodiPy (which has also been accepted to A&A) where we describe in more detail modelling of zodiacal emission and the approach taken by ZodiPy.

ZodiPy uses Astropy both in the public API and internally. When using one of the methods provided to simulate the zodiacal emission, the user needs to use Quantity and Time types in input arguments such as frequency/wavelength, angles on the sky, and the time of observation. Internally, the Time object is used to compute the heliocentric ecliptic position of the Earth and the observer with the SkyCoord and other functionality in astropy.coordinates.

@dhomeier
Copy link
Contributor

dhomeier commented Jul 6, 2022

Thank you for proposing this package as an affiliated package! I'm happy to confirm that your package is now under review, and we will post the results of the review here and on the mailing list.

@dhomeier
Copy link
Contributor

@MetinSa: apologies for leaving your proposal in a dormant state for so long!
In part this was due to a transition in our review system we were making over the last year, and which took also longer than foreseen to complete.
We now expect to have a review for your package within the next week or two.
But since we are at this point, I would also like to some details of the migration of our Affiliated Package review process to a new system in partnership with pyOpenSci now described in APE 22.

What this means for all packages such as yours, which have applied for Affiliated status before APE 22 was passed is, that we still offer you to continue the package review in the previous mode, but also want to give you the option to complete this process through the pyOpenSci page.

In that case we would have to find a second reviewer, but the (open) pyOpenSci review is generally expected to take not more than 3 weeks, and reviews and responses will be openly discussed as pyOpenSci GitHub issues as described in their submission guidelines. Your package would also be checked for satisfying the pyOpenSci guidelines in addition to Astropy-specific requirements such as integration into the Astropy ecosystem, but largely the criteria will be what we also expect for the Astropy Affiliated packages.
As a benefit this would result in

  1. additional listing as a pyOpenSci package
  2. automatic submission for publication in JOSS

Please comment here on your preferences, or if you need additional information on this process.
Thanks!

@dhomeier
Copy link
Contributor

dhomeier commented Feb 23, 2024

@MetinSa I am happy to tell you that this package has now been reviewed for inclusion in the Astropy affiliated package
ecosystem by a member of the Astropy community, and I have synthesised the results of the review here.

You can find out more about our review criteria in Reviewing affiliated packages.
For each of the review categories below we have listed the score and have
included some comments when the score is not green.

Functionality/Scope Specialized package
ZodiPy looks like a very healthy package. Highlighted below are some spots where there is room for improvement.
Integration with Astropy ecosystem Orange
It could have much more integration with Astropy core and Astropy affiliated packages. Aside from Astropy `units`, the public API does not have much integration with Astropy. Here are some issues:
  • The get_emission_ang method basically depends on two coordinates that define the ray from the observer to the direction in which they are looking (plus a time and a wavelength). The vertex of the ray can be specified in Cartesian coordinates and the direction of the ray in polar coordinates. Internally, the method instantiates SkyCoord objects in order to do positional astronomy calculations. It would be ideal if the user could specify both coordinates using SkyCoord instances. That would unify 5 arguments (theta, phi, obs, obs_pos, lonlat) into only two.
  • Conversion between coordinate frames is done using healpy.Rotator but could be done more simply and idiomatically with SkyCoord frame transformations.
  • There is a synthetic photometry feature to integrate the observed flux over a bandpass. ZodiPy could employ the Astropy affiliated package synphot here.
  • ZodiPy can evaluate the emission on HEALPix maps, but uses healpy to do this. It could use the Astropy affiliated package astropy-healpix here.
  • ZodiPy can plot emission components on all-sky maps, but uses healpy to do this. It could use reproject +
    astropy-healpix here for more flexible plotting.
Documentation Green
Testing Green
Development status Green
Some additional notes and suggestions from the reviewer follow:

The approaches to Numpy vectorization and Python multiprocessing parallelism are somewhat fixed function, and may not be performant for all use cases.

  • Methods do not support Numpy broadcasting for arbitrary input shapes. For example, it appears that obs_time must be scalar. For another, this line in get_obs_and_earth_positions discards the shape of the input arguments:
   return obs_position.reshape(3, 1, 1).value, earth_position.reshape(3, 1, 1).value
  • ZodiPy supports parallelism using Python multiprocessing, but it creates its own process pool and discards it when it is done. It would be nice if it supported the pool being passed by the user so that the cost of spawning child Python interpreters is not borne repeatedly. Also there are situations where it is necessary to use a custom Pool subclass (e.g. billiard).
Python 3 compatibility Green

Summary/Decision: With the overall positive review above your package already meets our legacy criteria for Astropy Affiliated packages, so I can offer you at this point to list your package on https://www.astropy.org/affiliated/.
I still encourage you to proceed with the new pyOpenSci application process by submitting this application as is, following their author guide; i.e. simply enter the relevant info in the submission form. At that point we would already have one review, and as soon as a second reviewer has completed their report (and potential comments been addressed), your package would be listed with pyOpenSci and JOSS as well.

@MetinSa MetinSa mentioned this pull request Feb 24, 2024
32 tasks
@MetinSa
Copy link
Contributor Author

MetinSa commented Feb 24, 2024

@dhomeier no worries! Thank you for taking your time to review ZodiPy and for the great feedback. All the listed issues seem like good improvements to the package and I will attempt to fix/implement them as soon as possible.

I would be very happy to have ZodiPy listed as an Astropy affiliated package. I also want to proceed with the the pyOpenSci application and have now submitted the following form where i mention this review.

Let me know if there is something regarding the pyOpenSci application that i have missed.
Thank you for the help!

@pllim
Copy link
Member

pllim commented Mar 6, 2024

@dhomeier , does this mean you want to add ZodiPy to the "legacy" listing as well, before I freeze it over at #573 ?

@pllim pllim changed the title Proposal to add ZodiPy to affiliated packages Proposal to add ZodiPy to affiliated packages (pyOpenSci/software-submission#161) Mar 6, 2024
Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this can go into the legacy section as there is no further action required on the Astropy side.

@pllim
Copy link
Member

pllim commented Mar 6, 2024

Please remember to use "squash and merge". Thanks!

@dhomeier dhomeier merged commit 202442f into astropy:main Mar 6, 2024
1 check passed
pllim added a commit to pllim/astropy.github.com that referenced this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants