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

HIERARCH + CONTINUE doesn't work. #358

Closed
rmjarvis opened this issue Jan 24, 2023 · 7 comments · Fixed by #398
Closed

HIERARCH + CONTINUE doesn't work. #358

rmjarvis opened this issue Jan 24, 2023 · 7 comments · Fixed by #398

Comments

@rmjarvis
Copy link
Contributor

fitsio can read/write long value items into a header using CONTINUE lines. And it can read/write long keyword names with HIERARCH lines. However, combining the two fails. Here is a quick unit test, which fails:

import fitsio
import numpy as np

header={
    "short1": "Short string",
    "short2": "A value string that is very long.  It needs to be at least 80 characters to trigger using CONTINUE, which this is.",
    "long_keyword_name1": "Another short string",
    "long_keyword_name2": "Another very long string that again must be at least 80 characters.  Probably less this time since it also uses HIERARCH."
}

with fitsio.FITS('temp.fits', 'rw', clobber=True) as f:
    f.write(np.array([1,2,3]), header=header)

with fitsio.FITS('temp.fits') as f:
    header2 = f[0].read_header()

for key in header:
    print(f"{key}\n    {header[key]}\n    {header2[key]}\n")
    assert header[key] == header2[key]

Output:

short1
    Short string
    Short string

short2
    A value string that is very long.  It needs to be at least 80 characters to trigger using CONTINUE, which this is.
    A value string that is very long.  It needs to be at least 80 characters to trigger using CONTINUE, which this is.

long_keyword_name1
    Another short string
    Another short string

long_keyword_name2
    Another very long string that again must be at least 80 characters.  Probably less this time since it also uses HIERARCH.
    Another very long string that again must be at le

Traceback (most recent call last):
  File "/Users/Mike/GalSim/tmp.py", line 19, in <module>
    assert header[key] == header2[key]
AssertionError

Note: This came up on LSST Slack (see thread on software-dev channel Jan 24, 10:28 AM EST). It seems to be a bug in the backend cfitsio implementation. Tim Jenness sent an email to cfitsio devs to report the issue. I'm not sure if it's possible for fitsio to work around this or not to fix the bug sooner, rather than just wait for cfitsio to fix it.

@timj
Copy link

timj commented Jan 24, 2023

In my quick test with Rubin code using cfitsio I ended up with a header that looks like this:

NOSPACE = 'test1   '
HIERARCH WITH SPACE = 'test2   '
LONGKWD = '0123456789012345678901234567890123456789012345678901234567890123456&'
CONTINUE  '7890    '
HIERARCH LONGKWD SPACE= '012345678901234567890123456789012345678901234567890123'
CONTINUE  '1234567890'

so the & is missing from the end and 7 characters have been dropped. I sent an email to the cfitsio help desk.

@beckermr
Copy link
Collaborator

Fitsio bundles cfitsio due to some features around Unicode strings that have not been upstreamed. If you have a patch that fixes the bug, we can add it to the patches we carry locally.

@esheldon
Copy link
Owner

Any news on this one?

@timj
Copy link

timj commented Jun 11, 2024

I haven't tried v4.4.0 of cfitsio but the release notes on https://heasarc.gsfc.nasa.gov/FTP/software/fitsio/c/docs/changes.txt mention long keywords but not HIERARCH so I don't know if it got fixed. I definitely didn't get an email about it from them and I sent a follow up to my email from 18 months ago.

@timj
Copy link

timj commented Jun 11, 2024

I am told this was fixed in cfitsio 4.3.0:

Enhancement of long string keyword read/write functions to fully conform with FITS standard specifications for multi-line value and comment strings. Two new functions have been added to implement this: fits_get_key_com_strlen and fits_read_string_key_com.

@esheldon
Copy link
Owner

I'll look into bundling 4.4.0 and using the new functions

@esheldon esheldon mentioned this issue Jun 12, 2024
@esheldon
Copy link
Owner

Moving to 4.4.0 fixed the bug without any changes in this package

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 a pull request may close this issue.

4 participants