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

Fix ci failures and update latest/min versions #779

Merged
merged 8 commits into from
Dec 14, 2023

Conversation

BradleySappington
Copy link
Collaborator

Updating versions and tests to handle new photutils version 1.10.0

@BradleySappington
Copy link
Collaborator Author

@obi-wan76 @mperrin @larrybradley -
Updated tests to handle new grid_xypos addition from photutils.
Looks like everything is okay except for test_psfgrid.py::test_saving.

Attaching the output from the failed comparison that checks a saved file with what was originally written.
(test_array1.txt is from the file write)

test_array1.txt
test_array2.txt

@BradleySappington
Copy link
Collaborator Author

@obi-wan76 @mperrin - When saving model, to maintain xygrid sorting, now writing using model.data instead of psf_array. Please advise if there are any science related issues with this change in particular.

@BradleySappington BradleySappington self-assigned this Dec 11, 2023
@BradleySappington BradleySappington changed the title DRAFT: Fix ci Fix ci failures and update latest versions Dec 11, 2023
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b45e76f) 54.52% compared to head (b99e0d0) 54.52%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #779   +/-   ##
========================================
  Coverage    54.52%   54.52%           
========================================
  Files           16       16           
  Lines         6560     6560           
========================================
  Hits          3577     3577           
  Misses        2983     2983           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BradleySappington BradleySappington changed the title Fix ci failures and update latest versions Fix ci failures and update latest/min versions Dec 11, 2023
@mperrin
Copy link
Collaborator

mperrin commented Dec 11, 2023

@obi-wan76 @mperrin - When saving model, to maintain xygrid sorting, now writing using model.data instead of psf_array. Please advise if there are any science related issues with this change in particular.

I think what we really need is @larrybradley to advise on interface issues with the newest version of photutils and the save/read functionality there. I'm not familiar enough in detail with the changes made in x,y order to know if this would cause any problems or not.

One small piece of advice though, is that if we're making changes in how the PSF grid files are saved, let's make sure that there's a version number keyword that's being saved into the files as metadata, so that when reading in any file it's 100% unambiguous which version of the software it was created with (and thus which x,y ordering). The science-related issue I can imagine would be reading in a saved file created with some software version X into code running version Y, and being confused about data ordering in such a way that the data in the PSF grid for field-dependence gets mixed up. Want to make sure that doesn't happen accidentally for any users!

Copy link
Collaborator

@mperrin mperrin left a comment

Choose a reason for hiding this comment

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

I have tested this PR and verified that

  1. If you create a psf grid, and save it to disk then read back in, then the computed grid and read-from-disk-file grid match precisely (as expected)
  2. The PSF in a given grid position matches the expected PSF for that detector location as given by the grid_xypos metadata. (I.e. the reordering of grid data in Fix GriddedPSFModel plot_grid astropy/photutils#1661 did not introduce any accidental surprises when making or reading grids using WebbPSF).

Tested with photutils 1.10.0.

LGTM.

@BradleySappington
Copy link
Collaborator Author

@mperrin , thank you for the review. Would you still like me to add the version of webbpsf in the header information?

@mperrin
Copy link
Collaborator

mperrin commented Dec 13, 2023

I checked and the version of WebbPSF is already in the header information!

@mperrin
Copy link
Collaborator

mperrin commented Dec 14, 2023

@obi-wan76 would you also want to review this? I see @BradleySappington tagged both of us. Or one of us can go ahead and merge now. Your choice. I think it would be nice to ge this merged sooner rather than later so the CI is working again and can be used to test all the other PRs...

Copy link
Collaborator

@obi-wan76 obi-wan76 left a comment

Choose a reason for hiding this comment

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

This looks good to me, specially after Marshall testing.

@obi-wan76 obi-wan76 merged commit 3c86f1d into spacetelescope:develop Dec 14, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants