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

FITMETHOD bytes vs. str #304

Merged
merged 1 commit into from
May 1, 2024
Merged

FITMETHOD bytes vs. str #304

merged 1 commit into from
May 1, 2024

Conversation

sbailey
Copy link
Collaborator

@sbailey sbailey commented May 1, 2024

This PR is a followup to #303 which broke the ability to write the rrdetails HDF5 file, due to a str-vs-bytes issue on the FITMETHOD column. Forcing FITMETHOD to .astype('S4') left the FITMETHOD column as bytes while the SPECTYPE and SUBTYPE columns were unicode strings. Writing to the fits file worked fine because astropy.io.fits is forgiving about bytes vs. str, but h5py is not and we tripped over our attempt to standardize it for h5py np.char.encode(zfit[colname], 'ascii').

This fix is two-fold:

  • use .astype('U4') instead of .astype('S4') so that FITMETHOD is a str column just like SPECTYPE and SUBTYPE
  • while writing the hdf5 file, first check if a unicode-to-bytes conversion is needed before trying to convert a column.

Either fix alone would have been sufficient; including both helps us be more future proof to other bytes-vs-str bugs.

I'm going to self-merge this so that I can proceed with Jura testing (otherwise zproc jobs are crashing when using redrock/main)

@coveralls
Copy link

Coverage Status

coverage: 39.04% (-0.01%) from 39.051%
when pulling 61b6603 on bytes_str_grr
into f7f95e5 on main.

@sbailey sbailey merged commit 3ddcf89 into main May 1, 2024
12 checks passed
@sbailey sbailey deleted the bytes_str_grr branch May 1, 2024 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants