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

Hfile pycbc version information check #4768

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

GarethCabournDavies
Copy link
Contributor

Add a pycbc_version attribute when opening a HFile to write, and check that it exists/matches when opening for reading.

Standard information about the request

This is a new feature
This change (can) affect all codes which use h5py input/output, if wanted
This changes warnings seen when running the code, no scientific content or results plotting will be affected

This change follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

I can add unit tests if desired

This change will warn when different versions of PyCBC are being used

Motivation

When the code changes, there are (sometimes unwitting) differences in how the files from the code are output/input. This just adds a warning to the user to be wary of that.

Contents

When a HFile object is opened, the file read/write permission will be used to determine whether or not to check / update the pycbc_version attribute.

  • If this version does not match the one being used, a warning is shown.
  • If using append permission, the version is changed, and a warning is shown

Testing performed

Tested each case (same / different versions, and unversioned files), and the correct warnings are given each time.
Tested that the permissions are carried through to the underlying h5py.File, and appropriate errors are raised.

  • The author of this pull request confirms they will adhere to the code of conduct

@GarethCabournDavies
Copy link
Contributor Author

Note that the changes to add_statmap are so that this is checked in the CI - codes should change to use HFile rather than h5py.File gradually, but I won't change them all in one go

@ahnitz
Copy link
Member

ahnitz commented May 31, 2024

Is there any more information on the motivation for this PR? I'm not sure how I feel about linking version of the pycbc library information into a file like this.

@GarethCabournDavies
Copy link
Contributor Author

@ahnitz sorry, I should have added you as a reviewer to get your opinion as well, as it is a proposal at the moment

Basically as different pycbc versions may not be able to work together, I thought it best to warn about this rather than anyone expecting the codes to work across different versions.

This would additionally help with code review as the reviewer could check that the pycbc version in the result files matches the one which should have been run

@GarethCabournDavies
Copy link
Contributor Author

GarethCabournDavies commented Jun 6, 2024

Note that the general option of pycbc always using HFile as a wrapper would go someway towards solving #1525

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