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

replace asdf.fits_embed #199

Merged
merged 4 commits into from Nov 6, 2023
Merged

replace asdf.fits_embed #199

merged 4 commits into from Nov 6, 2023

Conversation

braingram
Copy link
Contributor

asdf 3.0 removed the deprecated AsdfInFits (and asdf.fits_embed) used here to read/write fits files (see https://asdf.readthedocs.io/en/3.0.1/asdf/whats_new.html#whats-new-3-0-0 for descriptions of other changes).

This PR replaces uses of asdf.fits_embed with equivalent code. This was tested locally by modifying:

  • test_grid_io
  • test_field_io
  • test_mode_basis_io
    To save all test files (and not remove them after the test passed). Test files were generated with hcipy main and asdf 2.15.2 (we'll call these 'old' files) and with hcipy using this PR and asdf 3.0.1 ('new' files). hcipy main with asdf 2.15.2 could read both new and old files as could hcipy with this PR and asdf 3.0.1.

If there are static files that aren't captured by these tests it would be helpful to test those files with this PR to verify that the updated code can open all files generated with older versions of hcipy.

@ehpor
Copy link
Owner

ehpor commented Nov 3, 2023

Seems like there are some problems with files still being open after the write function has returned, but only on Windows.

@ehpor ehpor self-requested a review November 3, 2023 20:12
@ehpor ehpor added the bugfix label Nov 3, 2023
@ehpor ehpor self-assigned this Nov 3, 2023
@ehpor
Copy link
Owner

ehpor commented Nov 3, 2023

As discussed offline with Brett, the failing tests are likely caused by memmap handling changes between Windows and Linux/Mac. I'll try to fix this since the changes are most likely easy to fix, plus easy to combine with reviewing. If they are difficult to resolve, the ball is back in Brett's court.

@ehpor
Copy link
Owner

ehpor commented Nov 6, 2023

@braingram I don't know why codecov reports are not coming through to the Github check, but it's positive diff coverage, so I'm fine with force merging. Everything is working now as well.

image

Copy link
Owner

@ehpor ehpor left a comment

Choose a reason for hiding this comment

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

LGTM and tested very well.

@ehpor
Copy link
Owner

ehpor commented Nov 6, 2023

Force merging since codecov didn't forward its check to Github Checks for some reason. See comment above.

@ehpor ehpor merged commit 1e60fac into ehpor:master Nov 6, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants