Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Removed xml dependence from pycbc_pygrb_page_tables #4649

Merged
merged 80 commits into from
Apr 23, 2024
Merged

Removed xml dependence from pycbc_pygrb_page_tables #4649

merged 80 commits into from
Apr 23, 2024

Conversation

MarcoCusinato
Copy link
Contributor

  • Removed xml dependence from pycbc_pygrb_page_tables
  • Added hdf data extraction from hdf multi inspiral banks in pygrb_postprocessing_utils
  • Added quantities needed for postprocessin in pycbc_multi_inspiral

@MarcoCusinato MarcoCusinato changed the title WIP Removed xlm dependence from pycbc_pygrb_page_tables Removed xlm dependence from pycbc_pygrb_page_tables Mar 7, 2024
@MarcoCusinato MarcoCusinato marked this pull request as ready for review March 7, 2024 09:25
@pannarale pannarale added the PyGRB PyGRB development label Mar 7, 2024
@pannarale pannarale changed the title Removed xlm dependence from pycbc_pygrb_page_tables Removed xml dependence from pycbc_pygrb_page_tables Mar 7, 2024
@pannarale pannarale self-requested a review March 11, 2024 02:09
@pannarale
Copy link
Contributor

@MarcoCusinato, please rebase this branch on master in order to fix the failing checks.

@MarcoCusinato
Copy link
Contributor Author

@MarcoCusinato, please rebase this branch on master in order to fix the failing checks.

Branch updated.

pannarale
pannarale requested changes Mar 12, 2024
View reviewed changes
@pannarale
Copy link
Contributor

Hi @MarcoCusinato, I started adding some requests for changes to start from. In general, though, I don't understand why this PR is so big. For example, what is the need for load_missed_found_injections? Other post-processing scripts are handling injections for plots already, so somehow this function must not be necessary. Another example are the changes to get_bestnrs which should not be part of getting page_tables to assemble tables in html files.

@titodalcanton
Copy link
Contributor

I suspect there may have been some "git incident" here. I am happy to discuss offline if helpful.

@pannarale
Copy link
Contributor

We discussed this PR on a call and it seems that @jakeb245 could help reduce its size. For example, I flagged load_missed_found_injections in a comment above which was written by @MarcoCusinato to load injections, but actually @jakeb245 uses injs = ppu.load_triggers(found_missed_file, vetoes) in here. Similarly, @jakeb245 already handled templates parameters via the template id, which is something flagged here by @titodalcanton.
So, it would be good for @jakeb245 to chime in simplify this PR and make the changes more minimal.

jakeb245
jakeb245 reviewed Mar 22, 2024
View reviewed changes
Copy link
Contributor

@jakeb245 jakeb245 left a comment

Choose a reason for hiding this comment

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

Ok, I've pointed out some things that could be reduced. It seems that we've gone about using HDF5 files in two different ways. It sounds pulling things directly from trigger files as needed is the way we're trying to do things now.

pannarale
pannarale requested changes Apr 21, 2024
View reviewed changes
pannarale
pannarale requested changes Apr 21, 2024
View reviewed changes
pannarale
pannarale requested changes Apr 23, 2024
View reviewed changes
@MarcoCusinato
Copy link
Contributor Author

Tested, data and results at /home/marco.cusinato/page_tables_testing.

MarcoCusinato and others added 2 commits April 23, 2024 16:34
@MarcoCusinato @pannarale
Update bin/pygrb/pycbc_pygrb_page_tables
9d30049 
Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>
@MarcoCusinato
Removed duplicate print
07a90b4
@pannarale
Copy link
Contributor

Tested, data and results at /home/marco.cusinato/page_tables_testing.

Just checked them and they look good, thanks.

@pannarale pannarale merged commit a54f592 into gwastro:master Apr 23, 2024
33 checks passed
@pannarale pannarale mentioned this pull request Apr 23, 2024
Closed
7 tasks
ArthurTolley pushed a commit to ArthurTolley/pycbc that referenced this pull request May 21, 2024
@MarcoCusinato @pannarale @ArthurTolley
Removed xml dependence from pycbc_pygrb_page_tables (gwastro#4649)
bb7e20f 
* Sync pycbc_multi_inspiral

* Sycn page_tables

* Sync ppu

* Added back required=True

* Removed ported SNR from glue

* Addressed code climate

* Reverted changes in multi_inspiral

* Copied Jacob's sort_trigs function

* get_bestnrs reverted back, used slide_id in extract basic trig properties

* Modified extract_basic_trig_properties

* Modified extract_basic_trig_properties

* Reworked trigs

* Used hdf directly for the onsoyrce file

* Removed load_trig_data

* Moved load_missed_found_injections to page_tables executable

* Closed hdf files and completed injections

* Moved load_missed_found_injection

* Removed load_injections

* Changed for in recovered params

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* changed for in filtrer_stats

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Fixed loops

* Added function to calculate distance and chirp mass

* Removed reconstructed ra and dec

* Addressed codeclimate

* Added recovered spin

* Added tc to inj_params

* Added tc to inj_params

* Added ra and dec to on_trigs

* Added ra and dec to off_trigs

* Removed closing twice trig_data

* Added formatting for offsource trigs

* Added logging info

* Added recovered ra dec for found injections

* removed / from subsets

* Added 'rec_ra', 'rec_dec' at line 774

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Removed `,`at line 769

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Removed `opts` at line 582

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Removed spaces at line 643

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Added comment about missed injections

* Removed get_bestnrs

* Removed vetoes function

* Reverted back to previous extract basic trig properties

* Used reweighted snr correctly

* Removed spaces

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Changed comment

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Fixed typo

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Changed comment

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Added function description

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Removed comment ender

* Added and removed empty lines

* Update pycbc_pygrb_page_tables

Indentation

* Removed unused import

* Fixed issue with slide_id

* Addressed issues with hdf arrays slicing

* Addressed dictionary key issues

* Update bin/pygrb/pycbc_pygrb_page_tables

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Update bin/pygrb/pycbc_pygrb_page_tables

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Used reweightedsnr_cut

* Addressed codeclimate

* Removed trig_data from load_missed function

* Removed comments

* Changed bank file file name

* Removed try

* Added Delta t column

* Added Delta t column

* Cleaned unused options

* Removed bestnr options

* Cleaned stored options

* Corrected typo

* Update bin/pygrb/pycbc_pygrb_page_tables

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>

* Removed duplicate print

---------

Co-authored-by: Francesco Pannarale <francesco.pannarale@ligo.org>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@titodalcanton titodalcanton titodalcanton left review comments

@jakeb245 jakeb245 jakeb245 left review comments

@pannarale pannarale pannarale approved these changes

Assignees

@jakeb245 jakeb245

@MarcoCusinato MarcoCusinato

Labels
PyGRB PyGRB development
Projects
PyGRB Development
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MarcoCusinato @pannarale @titodalcanton @jakeb245