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

Standardise logging: bin/all_sky_search and pycbc/events #4576

Merged

Conversation

GarethCabournDavies
Copy link
Contributor

@GarethCabournDavies GarethCabournDavies commented Dec 1, 2023

Some changes to how logging is handled, this will grow to a bigger change over the coming weeks as I start to add more executables and modules into this. This MR contains executables in bin/all_sky_search and modules in pycbc/events:

  1. The first change is to include the logging level in the default logging format
  2. I plan to use init_logging in the executables as much as possible / where it is appropriate in order to keep logging the same throughoput the code
  3. The last change is one I wanted to check, see below

I have made this PR early as I thought it best to check that my plan was agreeable to others before going through all parts of the code.

One of the things I was going to change is that each module gets its own named logger (using this standard, one can then specify that logger to be more or less verbose when the module is called - this is useful for people downstream of us and is fairly standard practice from what I can see (people can even just set 'pycbc' to set things for all from pycbc which is nice). I have given an example from the cuts and significance modules.

@GarethCabournDavies GarethCabournDavies marked this pull request as draft December 1, 2023 15:22
Comment on lines 92 to 95
logging.info("A code-level logging info statement")
logging.debug("A code-level logging debug statement")
trigger_cut_dict, template_cut_dict = cuts.ingest_cuts_option_group(args)
logging.getLogger('pycbc.events.cuts').setLevel(logging.INFO)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this as an example of its usage (in the "REVERT ME..." commit so it can be cut out easily again).
The output of which (when running with verbose logging in pycbc_coinc_findtrigs) is:

2023-12-01T07:15:59.620-08:00 INFO : A code-level logging info statement
2023-12-01T07:15:59.620-08:00 DEBUG : A code-level logging debug statement
2023-12-01T07:15:59.620-08:00 INFO : A module-level logging info statement
2023-12-01T07:15:59.621-08:00 DEBUG : A module-level logging debug statement
2023-12-01T07:15:59.621-08:00 INFO : A module-level logging info statement
2023-12-01T07:15:59.638-08:00 INFO : Analyzing template 0 - 1158

We see that the module-level debug statement is not given on the second pass, as the cuts logger has been reset to INFO level

@GarethCabournDavies
Copy link
Contributor Author

Tagging @ahnitz @spxiwh @titodalcanton @cdcapano @tdent in case anyone has opinions on this before I start properly

@GarethCabournDavies GarethCabournDavies removed the request for review from spxiwh December 1, 2023 15:26
@titodalcanton
Copy link
Contributor

This will heavily affect some things in PyCBC Live, like the lag plotter. Need not forget this!

@GarethCabournDavies
Copy link
Contributor Author

This will heavily affect some things in PyCBC Live, like the lag plotter. Need not forget this!

That's exactly the situation I thought of with "where it is appropriate" - if I leave the pycbc_live executable alone, that should be okay

@GarethCabournDavies GarethCabournDavies marked this pull request as ready for review December 4, 2023 16:41
@GarethCabournDavies GarethCabournDavies changed the title Standardise logging Standardise logging: bin/all_sky_search and pycbc/events Dec 5, 2023
@GarethCabournDavies
Copy link
Contributor Author

As the verbose option was being set differently by different codes, I've also added a common_options function to __init__.py, which is called by each executable to set certain options as standard

It's named purposely vaguely so that we can add e.g. the version information to this later.

@GarethCabournDavies
Copy link
Contributor Author

Poke on this

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.

Looks good to me.

@GarethCabournDavies GarethCabournDavies merged commit 1906013 into gwastro:master Jan 26, 2024
31 checks passed
spxiwh pushed a commit to spxiwh/pycbc that referenced this pull request Feb 2, 2024
* add level to default logging

* Decrease logging level for debug information in coinc_findtrigs

* decrease logging level for some information in sngls_findtrigs

* add named logger in pycbc/events/cuts.py

* add named logger in pycbc/events/significance.py

* REVERT ME: an example of usage

* CC fix

* Revert "REVERT ME: an example of usage"

This reverts commit eb647e0.

* Use init_logging throughout all_sky_search

* give pycbc/events modules named loggers

* missed the fact that the level was set to warn, not info, as default before

* CC

* missed an import

* start moving verbose argparse into common facility

* add common_options to all_sky_search executables

* set --verbose default value to zero

* pycbc not imported in some codes

* CC

* rename function to be more decriptive
@GarethCabournDavies GarethCabournDavies added the v23_release_branch PRs applied to the v2.3.X release branch or to be cherry-picked if merging to master label Feb 2, 2024
@spxiwh spxiwh removed the v23_release_branch PRs applied to the v2.3.X release branch or to be cherry-picked if merging to master label Feb 2, 2024
spxiwh added a commit that referenced this pull request Feb 2, 2024
* Fix error querying Virgo frame type (#4523)

* Fix error querying Virgo frame type

* Typo

* Typo

* Implement Ian's suggestion

* Make it work

* Use an actual DeprecationWarning

* Scripts for dealing with preparation of offline gracedb upload files (#4534)

* Scripts for dealing with preparation of offline gracedb upload files

* Update bin/all_sky_search/pycbc_make_bayestar_skymap

Co-authored-by: Tito Dal Canton <tito@dalcanton.it>

* reinstate SNR timeseries being saved into HDF

* TDC comments

* TDC comments 2

* Remove unneeded import

---------

Co-authored-by: Tito Dal Canton <tito@dalcanton.it>
Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr>

* Use vetoed times followup in example (#4597)

* Use vetoed times followup in example

* Fix statistic files double-definition

* Rationalize some calls to waveform properties (#4540)

* rationalize some calls to waveform properties

* Only allow explictly listed names

* don't try to change function name

* g

g

* Add the ability to choose how to sort injections in minifollowups (#4602)

* Add the ability to choose how to sort injections in minifollowups

* Minor cleanup

* Minor refactor

* Tom's comments

* Update example search workflow

* Further thoughts and suggestions

* Tom's suggestion

* Fix bug and Tom's suggestion

* Standardise logging: bin/all_sky_search and pycbc/events (#4576)

* add level to default logging

* Decrease logging level for debug information in coinc_findtrigs

* decrease logging level for some information in sngls_findtrigs

* add named logger in pycbc/events/cuts.py

* add named logger in pycbc/events/significance.py

* REVERT ME: an example of usage

* CC fix

* Revert "REVERT ME: an example of usage"

This reverts commit eb647e0.

* Use init_logging throughout all_sky_search

* give pycbc/events modules named loggers

* missed the fact that the level was set to warn, not info, as default before

* CC

* missed an import

* start moving verbose argparse into common facility

* add common_options to all_sky_search executables

* set --verbose default value to zero

* pycbc not imported in some codes

* CC

* rename function to be more decriptive

* O4b idq offline update (#4603)

* Start offline dq workflow rework

* Modified stat

* Rewrote dq workflow

* Added note for future suggested changes to dq workflow

* Finished first draft of offline workflow

* Fixed a comment

* Removed unneeded argument to make dq workflow

* Made bin templates executable

* WOrking version of offline dq workflow

* Reverting non-functional changes in stat.py

* linting

* Reverted more non-functional changes

* Reverted low-latency related features

* Rearrange imports

* Addressed Ian's comments

* Simplified masking in dq workflow

* Modify dq workflow to avoid using numpy arrays

* Use vetoed times followup in example (#4597)

* Use vetoed times followup in example

* Fix statistic files double-definition

* Addressed more comments from Tom

* Addressed Gareth's comments on parser arguments

* Don't allow dq stat to uprank candidates

* Modify dq trigger rate plots to not use dq trigger rates below 1

* Address Tom's most recent comment

* Readded [Min 1] to dq plot's y-axis label

* Pass bank plotting tags to dq template bin plots

* Update bin/plotting/pycbc_plot_dq_flag_likelihood

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* Update bin/workflows/pycbc_make_offline_search_workflow

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* Use abs() correctly

* Break up 2 try/ecept cases

---------

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>
Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* Bumping version number

---------

Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr>
Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>
Co-authored-by: Tito Dal Canton <tito@dalcanton.it>
Co-authored-by: Thomas Dent <thomas.dent@usc.es>
Co-authored-by: maxtrevor <65971534+maxtrevor@users.noreply.github.com>
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
* add level to default logging

* Decrease logging level for debug information in coinc_findtrigs

* decrease logging level for some information in sngls_findtrigs

* add named logger in pycbc/events/cuts.py

* add named logger in pycbc/events/significance.py

* REVERT ME: an example of usage

* CC fix

* Revert "REVERT ME: an example of usage"

This reverts commit eb647e0.

* Use init_logging throughout all_sky_search

* give pycbc/events modules named loggers

* missed the fact that the level was set to warn, not info, as default before

* CC

* missed an import

* start moving verbose argparse into common facility

* add common_options to all_sky_search executables

* set --verbose default value to zero

* pycbc not imported in some codes

* CC

* rename function to be more decriptive
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
* add level to default logging

* Decrease logging level for debug information in coinc_findtrigs

* decrease logging level for some information in sngls_findtrigs

* add named logger in pycbc/events/cuts.py

* add named logger in pycbc/events/significance.py

* REVERT ME: an example of usage

* CC fix

* Revert "REVERT ME: an example of usage"

This reverts commit eb647e0.

* Use init_logging throughout all_sky_search

* give pycbc/events modules named loggers

* missed the fact that the level was set to warn, not info, as default before

* CC

* missed an import

* start moving verbose argparse into common facility

* add common_options to all_sky_search executables

* set --verbose default value to zero

* pycbc not imported in some codes

* CC

* rename function to be more decriptive
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
* add level to default logging

* Decrease logging level for debug information in coinc_findtrigs

* decrease logging level for some information in sngls_findtrigs

* add named logger in pycbc/events/cuts.py

* add named logger in pycbc/events/significance.py

* REVERT ME: an example of usage

* CC fix

* Revert "REVERT ME: an example of usage"

This reverts commit eb647e0.

* Use init_logging throughout all_sky_search

* give pycbc/events modules named loggers

* missed the fact that the level was set to warn, not info, as default before

* CC

* missed an import

* start moving verbose argparse into common facility

* add common_options to all_sky_search executables

* set --verbose default value to zero

* pycbc not imported in some codes

* CC

* rename function to be more decriptive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants