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' |
There was a problem hiding this comment.
Is this sufficient to run these tests in the CI? So far I see lots of --pyargs photutils in the CI jobs which result in unpredictable file paths which typically result in the pytest-asdf-plugin failing to find schemas to tests: asdf-format/pytest-asdf-plugin#8
It's unclear to me if it's helpful to test schemas as part of all CI jobs (for example oldestdeps or macos vs linux) so one thing to consider is delegating schema testing to 1 CI job (either an existing one or a new one).
FWIW if I run pytest locally it is picking up these tests. However I'm not familiar enough with the photutils CI to know what to expect here. For example, considering the py313-all-deps job:
Most of that difference appears to be from this PR having a base commit older than HEAD on main.
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.
| 'to_yaml_tree', # ASDF converter | ||
| 'from_yaml_tree', # ASDF converter |
There was a problem hiding this comment.
It seems useful to include these in the coverage measurement. Aren't they covered by the converter tests?
There was a problem hiding this comment.
These were added here to avoid running the numpydoc validation on them.
I could add docstrings but they won't be very useful.
| - tag_uri: tag:astropy.org:photutils/aperture/circular_aperture-1.0.0 | ||
| schema_uri: asdf://astropy.org/photutils/schemas/aperture/circular_aperture-1.0.0 | ||
| title: CircularAperture | ||
| description: ASDF schema for serializing a photutils CircularAperture. |
There was a problem hiding this comment.
| description: ASDF schema for serializing a photutils CircularAperture. | |
| description: Circular aperture with one or more positions. |
This is a nitpick. Since this is a tag definition I don't think "ASDF schema" fits. I'm also on the fence about adding "photutils" since that's already in the URI (and in other context). If this is updated would you update the other descriptions to match?
There was a problem hiding this comment.
By other descriptions did you mean the description in the schema file? I was under the impression that description needs to match the one in the manifest file but apparently not.
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. |
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