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 for Tabular-fits reader/writer fix #1113

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

kelle
Copy link
Member

@kelle kelle commented Nov 22, 2023

This PR modifies the tabular-fits writer to put the header information in BOTH HDU0 and HDU1 based on the conversation in #905 .

Other folks are welcome to modify this PR.

Broke out the testing of the update_header method into its own test.

fits compressed currently failing but I think that might be a file/memory handling thing which I may have accidentally broken in the test file.

Remaining Todo:

  • fix broken fits_compressed test
  • add more tests
  • add test that makes sure COMMENT and HISTORY keywords are perserved
  • make sure reader is doing what we expect
  • fix update_header behavior and tests.

@kelle
Copy link
Member Author

kelle commented Nov 22, 2023

At some point as part of the read method, the Header object gets converted into an OrderedDict. Not sure where that's happening...I don't see that in the tabular_fits_loader function.

@kelle
Copy link
Member Author

kelle commented Nov 22, 2023

I think the trouble is in Table.read which I guess changes everything to an ordered dictionary....FML.

@kelle
Copy link
Member Author

kelle commented Nov 22, 2023

Added this to the reader and all is now well

tab.meta = fits.getheader(file_obj, 0)

Comment on lines 75 to +78
tab = Table.read(file_obj, format='fits', hdu=hdu)

# Get the header from HDU0
tab.meta = fits.getheader(file_obj, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is breaking compression support since Table.read() in those cases closes the file_obj on return, so you can no longer read from it. Probably need to get away without using the table io at all here as well, i.e. use read_fileobj_or_hdulist as wcs1d_fits_loader does and read both tab and header from hdulist (not sure if Table directly supports this).

# Read it in and check against the original
with fits.open(tmpfile) as hdulist:
assert hdulist[0].header['NAXIS'] == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this no longer working? Would be worrying, because an "empty" PrimaryHDU should always have NAXIS = 0!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll put it back in!

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.

None yet

2 participants