Conversation
|
pre-commit.ci autofix |
|
pre-commit.ci autofix |
|
pre-commit.ci autofix |
|
Bumping the min version of astropy to v 7.2 fixed the oldestdeps test. Is this an acceptable change? |
|
@nden Is astropy 7.2 (released Nov 2025) the only version that works? Requiring only the latest release of astropy does seem to be a bit restrictive. That would also put constraints on all photutils dependencies, like jwst, romancal, drizzlepac/hap, etc. |
|
The failing tests were using astropy 6.1.4. It appears there was a follow up bug fix release, 6.1.7, which works. |
braingram
left a comment
braingram
left a comment
There was a problem hiding this comment.
I do not have enough experience with photutils to test if real instances of these classes roundtrip so I only looked at this from an asdf extension perspective. I left a few comments, questions and suggestions.
Overall looks like it's on the right track. Aside from the one suggested schema change the rest of the comments are mostly questions or test changes.
| filterwarnings = [ | ||
| 'error', # turn warnings into exceptions | ||
| ] | ||
| asdf_schema_root = 'photutils/resources/schemas' |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I rebased this PR on main. Now the number of tests is
- on main: 3038 passed, 30 skipped
- this pr: 3040 passed, 30 skipped
This is the expected change in number of tests counting the converter tests.
I was not aware of the issue of testing schemas with --pyargs. Thanks for the pointers. Commit 96f4d4b adds a dedicated test for the schemas.
photutils/resources/schemas/aperture/circular_aperture-1.0.0.yaml
Outdated
Show resolved
Hide resolved
try another way to fix olddeps test
|
@braingram Thanks for the review. I think I addressed the comments except one additional question. |
|
@larrybradley Unless you have other comments I think this is ready to merge. |
larrybradley
left a comment
larrybradley
left a comment
There was a problem hiding this comment.
Thanks, @nden (and @braingram for the review). Is there a longer-term plan to add more photutils objects? I'm planning to release 3.0 soon, and was wondering if this PR should be included.
|
@larrybradley I will submit two more PRs very soon, implementing serialization for the rest of the PSFs and the apertures. |
3 participants
This PR is a contribution to astropy/astropy-project#527 . It implements the initial setup for serializing photutils PSF and aperture objects to ASDF. The proposal mentioned the implementation will be in asdf-astropy. However, after discussion with the ASDF developers we decided to include the implementation in photutils in order to keep the library code in sync with the serialization more easily. This PR adds
I am not able to request reviewers so tagging here @larrybradley @perrygreenfield @braingram