Skip to content

Add logical_as_bytes parameter to preserve NULL values with logical columns in FITS#19374

Draft
astrofrog wants to merge 1 commit intoastropy:mainfrom
astrofrog:logical-as-bytes
Draft

Add logical_as_bytes parameter to preserve NULL values with logical columns in FITS#19374
astrofrog wants to merge 1 commit intoastropy:mainfrom
astrofrog:logical-as-bytes

Conversation

@astrofrog
Copy link
Member

@astrofrog astrofrog commented Mar 7, 2026 •
edited
Loading

This is an experimental fix for #18815

Essentially it:

  • adds an option logical_as_bytes which is analogous to character_as_bytes and when True will return logical columns as bytes
  • warns if a user is reading a file with logical columns and is not making use of the logical_as_bytes keyword argument:
In [1]: from astropy.io import fits

In [2]: hdu = fits.open('logical_null_test.fits')[1]

In [3]: hdu.data
Out[3]: WARNING: Column 'flag' contains NULL (undefined) values which will be converted to False. To preserve NULL information, reopen the file with logical_as_bytes=True and check for values of 0 (NULL), ord('T')=84 (True), and ord('F')=70 (False). [astropy.io.fits.fitsrec]

FITS_rec([(True,), (False,), (False,)],
         dtype=(numpy.record, [('flag', 'i1')]))

In [4]: hdu = fits.open('logical_null_test.fits', logical_as_bytes=True)[1]

In [5]: hdu.data
Out[5]: 
FITS_rec([(84,), (0,), (70,)],
         dtype=(numpy.record, [('flag', 'i1')]))

We can of course try and find another solution, but I thought it would be worth trying this out since it's not too complex in terms of lines changed (which includes the new test) and mirrors how data is read in with CFITSIO (see #18815 (comment)). There's also precedent for this kind of solution (character_as_bytes) and it is backward-compatible.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@astrofrog
Add logical_as_bytes parameter to preserve NULL values in FITS logica…
8088f94 
…l columns

The FITS standard specifies that logical columns can contain 'T' (True),
'F' (False), or 0x00 (NULL/undefined). Previously, astropy converted all
logical columns to numpy bool, which cannot represent the NULL state -
NULL values were silently converted to False.

This change adds a `logical_as_bytes` parameter (analogous to the existing
`character_as_bytes`) that can be passed to `fits.open()`. When True,
logical columns are returned as int8 arrays containing the raw byte values:
- 84 (ord('T')) for True
- 70 (ord('F')) for False
- 0 for NULL

Additionally, a warning is now issued when NULL values are present and
logical_as_bytes=False (the default), alerting users that NULL information
is being lost.
@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2026

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim added this to the v8.0.0 milestone Mar 7, 2026
@astrofrog
Copy link
Member Author

astrofrog commented Mar 7, 2026

As a side note, if we go with this approach we can also make it so that Table.read(...) does the right thing for these columns behind the scenes

@astrofrog
Copy link
Member Author

astrofrog commented Mar 7, 2026

Ok so interestingly this shows that at least one of our existing test files (tb.fits) actually contains such a column (which is then causing a warning in another test which is changed to an exception).

@saimn
Copy link
Contributor

saimn commented Mar 20, 2026

Thanks for diving into this issue @astrofrog :)
Indeed we could imagine a solution like this. Anything more user friendly seems probably too complicated to implement with our current resources (e.g. having a real mask for any column that needs it and that could be passed to Table directly).
However returning an array of [84, 0, 70] seems a bit obscure. Maybe using [b'T', b'', b'F'] values instead would make it more explicit that this is a logical column, as it is closer to what is actually stored ? Anyway the two versions are equivalent as one can easily be viewed into the other.
Also we should consider adding writing support at the same time, while we are at it.

@saimn
Copy link
Contributor

saimn commented Mar 20, 2026

Also the same solution should be used for VLA columns (#18755), it can be done in another PR but just mentioning it to remind that the chosen approach should be compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saimn saimn Awaiting requested review from saimn saimn will be requested when the pull request is marked ready for review saimn is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Experimental io.fits needs-discussion

Projects

None yet

Milestone

v8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@astrofrog @saimn @pllim