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

Fix error querying Virgo frame type #4523

Merged
merged 6 commits into from
Jan 17, 2024

Conversation

titodalcanton
Copy link
Contributor

@titodalcanton titodalcanton commented Oct 6, 2023

When using gwdatafind to query for a given frame type and channel name, the pycbc.frame module assumes that the frame type starts with the letter identifying the site (H, L, V etc). However it appears that this assumption is not always valid: for example, recent Virgo data seems to have changed the frame type to HoftOnline, making it impossible to use query_and_read_frame() to retrieve Virgo data.

This PR changes query_and_read_frame() so that the frame type (and therefore the --frame-type CLI option) is allowed to have a "S:" or "S?:" prefix, in which case S is taken as the site. For backward compatibility, this prefix can be omitted, in which case the site will be taken from the channel name instead. If that also does not start with the right prefix, the code prints a warning and takes the site from the first letter of the frame type, as it did in the past (and this will not work for Virgo O4 data).

@spxiwh
Copy link
Contributor

spxiwh commented Oct 9, 2023

Alternative proposal:

Allow supplying an explicit ifo in frame type. So something like:

--frame-type = V:HoftOnline

for consistency we should also allow the more obvious:

--frame-type = V1:HoftOnline

Not sure how this works for the multi-ifo options though. Do we then need to be able to handle:

--frame-type = V1:V1:HoftOnline H1:Hwhatever

.... some other options do similar things, and would let us pretend that Virgo data is actually from another ifo.

@titodalcanton
Copy link
Contributor Author

@spxiwh so basically you are proposing handling the frame type as we already handle the channel? That would include doing the same with respect to multi-ifo options, right?

@spxiwh
Copy link
Contributor

spxiwh commented Oct 9, 2023

@spxiwh so basically you are proposing handling the frame type as we already handle the channel? That would include doing the same with respect to multi-ifo options, right?

Basically, yes. I think there are a few differences between the two (as channel always has an ifo), but ... yes.

@ahnitz
Copy link
Member

ahnitz commented Dec 13, 2023

@titodalcanton What's the status of this PR? Is it still intended to be merged as is? Should I go ahead and review it?

@titodalcanton
Copy link
Contributor Author

I was going and try to implement @spxiwh's suggestion during the end of the year break.

@titodalcanton titodalcanton requested a review from spxiwh January 8, 2024 14:58
@titodalcanton
Copy link
Contributor Author

@spxiwh I think I was able to implement what you suggested, back to you for review. Not tested.

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.

I'm approving this, but I think this does not use the full potential of the MultiDetOptionActionSpecial where you could run with data from L1, but pretend it's V1.

args.frame_type[ifo],
args.start_time,
args.end_time,
site=ifo[0]
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 (unlike channel type) this would not work with the "lying" case of --frame-type H1:L:L_H00 or H1:L_RAW. Does this matter?

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 do not think this particular bit is that important. This concerns the StrainBuffer class, used by PyCBC Live only, and we do not normally get h(t) by specifying the frame type for PyCBC Live.

@titodalcanton
Copy link
Contributor Author

@spxiwh thanks, let's revisit what you noted in a later PR if the need arises.

@titodalcanton titodalcanton merged commit 438680d into gwastro:master Jan 17, 2024
32 of 33 checks passed
@titodalcanton titodalcanton deleted the fix_query_frame_for_virgo branch January 17, 2024 12:17
@titodalcanton titodalcanton added the v23_release_branch PRs applied to the v2.3.X release branch or to be cherry-picked if merging to master label Jan 17, 2024
spxiwh pushed a commit to spxiwh/pycbc that referenced this pull request Feb 2, 2024
* Fix error querying Virgo frame type

* Typo

* Typo

* Implement Ian's suggestion

* Make it work

* Use an actual DeprecationWarning
@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 <[email protected]>

* reinstate SNR timeseries being saved into HDF

* TDC comments

* TDC comments 2

* Remove unneeded import

---------

Co-authored-by: Tito Dal Canton <[email protected]>
Co-authored-by: Tito Dal Canton <[email protected]>

* 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 <[email protected]>

* Update bin/workflows/pycbc_make_offline_search_workflow

Co-authored-by: Thomas Dent <[email protected]>

* Use abs() correctly

* Break up 2 try/ecept cases

---------

Co-authored-by: Gareth S Cabourn Davies <[email protected]>
Co-authored-by: Thomas Dent <[email protected]>

* Bumping version number

---------

Co-authored-by: Tito Dal Canton <[email protected]>
Co-authored-by: Gareth S Cabourn Davies <[email protected]>
Co-authored-by: Tito Dal Canton <[email protected]>
Co-authored-by: Thomas Dent <[email protected]>
Co-authored-by: maxtrevor <[email protected]>
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
* Fix error querying Virgo frame type

* Typo

* Typo

* Implement Ian's suggestion

* Make it work

* Use an actual DeprecationWarning
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
* Fix error querying Virgo frame type

* Typo

* Typo

* Implement Ian's suggestion

* Make it work

* Use an actual DeprecationWarning
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
* Fix error querying Virgo frame type

* Typo

* Typo

* Implement Ian's suggestion

* Make it work

* Use an actual DeprecationWarning
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