Skip to content

Conversation

@pannarale
Copy link
Contributor

This PR is the eighth in the series started in PR #4929. Much like PRs #4947, #4950, #4955, and #4958, this PR provides a version of pycbc_pygrb_plot_injs_results that uses veto and segments files, as well as the utilities introduced recently to streamline the PyGRB results scripts.

Standard information about the request (and the following ones that will be linked to this)

This is a: a new feature enabling veto definer file usage in PyGRB. Utilities and scripts in results production are being streamlined along the way.

This change affects: PyGRB

This change changes: result presentation / plotting and scientific output.

Should this change break the standard automated test running --help for PyGRB plotting scripts, I will add some workarounds to avoid this. If needed, these will likely be empty functions: the plotting scripts will be progressively renovated in the whole series of PRs.

Motivation

Now that the workflow generator passes the veto definer file to the jobs where needed, its usage in the PyGRB results scripts is possible.

Testing performed

The totality of the changes that will be broken down in multiple PRs was tested on GRB 170817A data by producing a full results webpage (see here).

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

Sorry, something went wrong.

@pannarale pannarale added the PyGRB PyGRB development label Nov 26, 2024
@pannarale pannarale self-assigned this Nov 26, 2024
Copy link
Contributor

@MarcoCusinato MarcoCusinato left a comment

Choose a reason for hiding this comment

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

Minor comments, however I do not fully understand the name chenge of some functions.

input_file_handle[tag+'/mass1'][:],
input_file_handle[tag+'/mass2'][:])
injs[tag+'/mass1'],
injs[tag+'/mass2'])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need the [:]since we are working with numpy arrays.

Copy link
Contributor Author

@pannarale pannarale Nov 27, 2024

Choose a reason for hiding this comment

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

Actually, now that I look at it, I think the [:] are redundant everywhere. We used to operate with an h5 file handle (input_file_handle), but now we operate with injs which is dictionary produced by ppu.load_data that pairs keys to numpy arrays, so the [:] do not make a difference.

Here is a example of what I mean on a random h5 file. What we used to do:

>>> import h5py
>>> f = h5py.File("H1L1-EM_BRIGHT_BANK_MAXMBH25_NS0p05_ER10PSD-1163174417-604800.h5", "r")
>>> f.keys()
<KeysViewHDF5 ['approximant', 'f_lower', 'mass1', 'mass2', 'spin1z', 'spin2z', 'template_hash']>
>>> f['mass1']
<HDF5 dataset "mass1": shape (192772,), type "<f4">
>>> f['mass1'][:]
array([ 3.9331937,  3.9396975,  3.9436226, ..., 21.80699  , 24.884245 ,
       24.870836 ], dtype=float32)

so the [:] mattered. What ppu.load_data does now is

>>> tmp = {'a': f['mass1'][:]}
>>> tmp['a']
array([ 3.9331937,  3.9396975,  3.9436226, ..., 21.80699  , 24.884245 ,
       24.870836 ], dtype=float32)
>>> tmp['a'][:]
array([ 3.9331937,  3.9396975,  3.9436226, ..., 21.80699  , 24.884245 ,
       24.870836 ], dtype=float32)

so the [:] is redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, they should be removed everywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

You can make this function a bit more readable if you just define two local variables mass1 and mass2 at the very beginning, and then you avoid having to index injs again and again in the rest of the function.

pannarale and others added 2 commits November 27, 2024 09:46
Co-authored-by: Marco Cusinato <marco.cusinato.96@gmail.com>
Co-authored-by: Marco Cusinato <marco.cusinato.96@gmail.com>
@pannarale
Copy link
Contributor Author

Minor comments, however I do not fully understand the name chenge of some functions.

If you mean things like from load_ to complete_, my thought was that we do not actually load the chirp mass data for example (it is not in the results file), we compute it here, completing the request to have chirp mass information plotted.

"""Extract data related to mass ratio, chirp mass or total mass from raw
injection data"""

data = np.full(len(injs[tag+'/mass1'][:]), None)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line does not seem to be necessary.

@MarcoCusinato MarcoCusinato merged commit f82b6d1 into gwastro:master Nov 29, 2024
29 checks passed
@pannarale pannarale deleted the pygrb_vetoes_injs_results branch November 29, 2024 21:44
khunsang pushed a commit to khunsang/pycbc that referenced this pull request May 23, 2025
* Version of pycbc_pygrb_plot_injs_results

* Update bin/pygrb/pycbc_pygrb_plot_injs_results

Co-authored-by: Marco Cusinato <marco.cusinato.96@gmail.com>

* Update bin/pygrb/pycbc_pygrb_plot_injs_results

Co-authored-by: Marco Cusinato <marco.cusinato.96@gmail.com>

* Small optimization: awqvoid calculating eta and not using it

* Removing redundant slicings; introducing auxiliary vars for readability; fixing background array

* Removed redundant line

---------

Co-authored-by: Marco Cusinato <marco.cusinato.96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PyGRB PyGRB development

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

None yet

3 participants