View or index rather than explicitly set dtype and byteswap by mhvk · Pull Request #19585 · astropy/astropy · GitHub
Skip to content

View or index rather than explicitly set dtype and byteswap#19585

Draft
mhvk wants to merge 6 commits intoastropy:mainfrom
mhvk:io-fitsrec-do-not-set-dtype
Draft

View or index rather than explicitly set dtype and byteswap#19585
mhvk wants to merge 6 commits intoastropy:mainfrom
mhvk:io-fitsrec-do-not-set-dtype

Conversation

@mhvk
Copy link
Copy Markdown
Contributor

@mhvk mhvk commented Apr 22, 2026

This is a follow-up of #19584, to avoid using any dtype setting or inplace byte-swapping in io.fits.

Note that this also removes the untested private _writedata_by_row method, and blithely changes the private _get_heap_data() to do the swapping (but then, it cannot get pre-swapped data any more, since the _binary_table_byte_swap routine is removed as well).

Given that, I think this should be for 8.0, not 7.x.

Opening as draft while #19584 is being discussed.

  • 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.

@github-actions
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Contributor

@saimn saimn left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks @mhvk. My main concern is on the performance side, could you do some benchmark to see if there is an impact ? 🙏

# If the raw data was a user-supplied recarray, we can't write
# unicode columns directly to the file, so we have to switch
# to a slower row-by-row write
self._writedata_by_row(fileobj)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was added long ago (4327621) in the py2/py3 epoch, I guess it used to work but seems broken now (from_columns attempts to convert unicode arrays to bytes and fails).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you check if there is a performance impact on checksum computation ?

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.

3 participants