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

Proposed affiliated package: regularizePSF #528

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

jmbhughes
Copy link
Contributor

regularizePSF is a new tool made as part of the PUNCH mission to correct variable point-spread functions in images. It is applicable to any astronomical image with a variable PSF. It includes tools to model the variable PSF, regularize the PSF, and visualize results/intermediate steps. The PUNCH Science Operations Center team thinks it would be a good addition to the astropy affiliated package list.

@WilliamJamieson
Copy link
Contributor

Hi, sorry for any delays. We are starting the process of reviewing this package.

@WilliamJamieson
Copy link
Contributor

@taldcroft please feel free to add any comments you have.

@dhomeier
Copy link
Contributor

Uh, CI is picky about capitalisation. And how did the asdf-astropy change get in there – need to rebase perhaps?

@pllim
Copy link
Member

pllim commented Jun 30, 2023

Looks like this was rebased on the wrong branch or something? asdf-astropy is not supposed to be there in the diff.

@taldcroft
Copy link
Member

taldcroft commented Jul 2, 2023

Hi @jmbhughes -

Thanks for your submission, I have reviewed this affiliated package request from the astropy perspective.

Overall the package is very well done from the perspective of a modern well-structured Python package. The documentation is good, there is a supporting paper, testing seems good with 95% coverage. The code itself appears to be well-written and clearly documented.

I this package put it in the general purpose category since handling a spatially-dependent PSF is a fairly general task.

The only minor nitpick is that it uses lmfit for doing fitting where I suspect that astropy.modeling would work just as well. I wasn't previously familiar with lmfit but it seems quite similar to modeling. This is not a big deal and it might be that lmfit has needed features or maybe the authors were just used to using it.

I am happy to recommend accepting regularizePSF as an affiliated package.

cc: @WilliamJamieson

Copy link
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

@jmbhughes I am pleased to say that I accept this package as an affiliated package on the behalf of the astropy organization.

As far as the lmfit comments, while we would like our affiliated packages to use astropy as much as possible this is not necessarily required. Only that the package "integrates with the astropy ecosystem". I feel that this package does indeed integrate in the astropy ecosystem.

This being said, as one of the modeling maintainers, I would be happy to assist you with transitioning lmfit to using astropy.modeling. This includes making any bugfixes/changes to astropy.modeling to accommodate what might be necessary to do this so long as it does not break existing functionality. Please make an issue in astropy and/or regularizePSF and tag me in it if you would like to pursue this further.

@WilliamJamieson WilliamJamieson merged commit 2751cf3 into astropy:main Jul 3, 2023
"testing": "Good",
"devstatus": "Good",
"python3": "Yes",
"last-updated": "2023-04-21"
Copy link
Member

Choose a reason for hiding this comment

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

I thought this date is supposed to be the date of acceptance? Though, I cannot find explicit mention of that in https://github.com/astropy/astropy-project/blob/main/affiliated/affiliated_package_review_process.md . Do you remember, @hamogu or @dhomeier ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just recall this had been discussed before e.g. https://github.com/astropy/astropy.github.com/pull/489/files#r866905596
but prior usage probably indeed has generally referred to the last change of package status.
No one would want to keep the latest release dates up to date anyway...
But as long as it reflects, in this case, the date/version the review was based on, I think we can live with it.
Should probably clarify it in the docs, or in whatever documentation we are going to use from now on.

Copy link
Member

Choose a reason for hiding this comment

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

I think I have been using it as "acceptance date", see #490 😬

Copy link
Member

Choose a reason for hiding this comment

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

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.

5 participants