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

Run conversion on all hdf injection files, allowing LVK injection format hdf files to be used #4455

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

GarethCabournDavies
Copy link
Contributor

I don't know if the file specification changed, or if someone changed from py2 to py3 without thinking, but this attribute is bytes in some of the examples we have been given

@spxiwh
Copy link
Contributor

spxiwh commented Aug 8, 2023

Would be good to get a check from Tom or the Rates team in terms of what to expect here .... and to check nothing else has changed!

@GarethCabournDavies
Copy link
Contributor Author

I'm also hitting an error in https://github.com/gwastro/pycbc/blame/master/pycbc/inject/inject.py#L372 - where it uses [()], it seems like it should be [:] - in fact that's what it used to be?

@ahnitz
Copy link
Member

ahnitz commented Aug 10, 2023 via email

@GarethCabournDavies
Copy link
Contributor Author

[:] would be wrong as these should be scalars.

Okay, I think I was missing [inj2hdf] in the injection config file, so things weren't being converted, and it looked like an error in a different place

@GarethCabournDavies
Copy link
Contributor Author

Okay it is this line, which just returns the file without converting if it is a hdf file which we want to change as well

I don't know if the file specification changed, or if someone changed from py2 to py3 without thinking, but this attribute is bytes in some of the examples we have been given
@GarethCabournDavies
Copy link
Contributor Author

This now passes injections as appropriate to the inspiral jobs

@GarethCabournDavies
Copy link
Contributor Author

Would be good to get a check from Tom or the Rates team in terms of what to expect here .... and to check nothing else has changed!

I asked the rates team about this, they suggested to allow both bytes or string if we can

@@ -147,12 +147,9 @@ def cut_distant_injections(workflow, inj_file, out_dir, tags=None):
return node.output_files[0]

def inj_to_hdf(workflow, inj_file, out_dir, tags=None):
""" Convert injection file to hdf format if not already one
""" Convert injection file to hdf format, if it is already one,
this just makes a copy
Copy link
Contributor

Choose a reason for hiding this comment

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

Docstring naming convention says that line 1 should be a single sentence.

So this should really be:

""" Convert injection file to hdf format.

If the file is already PyCBC HDF format, this will just make a copy.
"""

Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

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

Thanks, merge when ready.

This PR also runs conversion on all HDF files, allowing us to actually use LVK injection files, which is more than just a metadata naming correction patch as the title now suggests.

@GarethCabournDavies GarethCabournDavies changed the title Allow bytes in LVK injection format hdf Run conversion on all hdf injection files, allowing LVK injection format hdf files to be used Aug 14, 2023
@GarethCabournDavies
Copy link
Contributor Author

Thanks, merge when ready.

This PR also runs conversion on all HDF files, allowing us to actually use LVK injection files, which is more than just a metadata naming correction patch as the title now suggests.

Good point - I have updated the title of the PR to reflect this

@GarethCabournDavies GarethCabournDavies enabled auto-merge (squash) August 14, 2023 10:30
@GarethCabournDavies GarethCabournDavies merged commit 583bfa3 into master Aug 14, 2023
71 of 72 checks passed
@GarethCabournDavies GarethCabournDavies deleted the lvk_inj_format_patch branch August 14, 2023 12:27
PRAVEEN-mnl pushed a commit to PRAVEEN-mnl/pycbc that referenced this pull request Nov 3, 2023
…mat hdf files to be used (gwastro#4455)

* Allow bytes in LVK injection format hdf

I don't know if the file specification changed, or if someone changed from py2 to py3 without thinking, but this attribute is bytes in some of the examples we have been given

* Dont assume a hdf is pycbc-style injections

* Update pycbc/workflow/injection.py
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
…mat hdf files to be used (gwastro#4455)

* Allow bytes in LVK injection format hdf

I don't know if the file specification changed, or if someone changed from py2 to py3 without thinking, but this attribute is bytes in some of the examples we have been given

* Dont assume a hdf is pycbc-style injections

* Update pycbc/workflow/injection.py
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
…mat hdf files to be used (gwastro#4455)

* Allow bytes in LVK injection format hdf

I don't know if the file specification changed, or if someone changed from py2 to py3 without thinking, but this attribute is bytes in some of the examples we have been given

* Dont assume a hdf is pycbc-style injections

* Update pycbc/workflow/injection.py
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