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

Idq live #4850

Merged
merged 15 commits into from
Aug 22, 2024
Merged

Idq live #4850

merged 15 commits into from
Aug 22, 2024

Conversation

maxtrevor
Copy link
Contributor

Standard information about the request

This update allows the DQExpFitFgBgNormStatistic ranking statistic to be used in the low-latency PyCBC Live Search

This change primarily affects code used in the live search. No difference is expected in the Live search unless the DQExpFitFgBgNormStatistic is chosen. The same statistic code is also used in the offline search, but this update should not change the results of using the DQ stat in offline.

This change changes scientific output of the low-latency search if DQExpFitFgBgNormStatistic is selected as the ranking statistic.

Contents

While this change allows the DQExpFitFgBgNormStatistic to be used in low-latency, it does not include code changes needed to produce a new statistic file for that stat to use.

I plan to make a separate merge request in the next few days building on #4813 that will allow a new statistic file to be produced and read in daily.

Testing performed

Ran on the O3 replay MDC for a day

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

@maxtrevor
Copy link
Contributor Author

Noting that this is the completed version of a previous draft MR: #4818

Copy link
Contributor

@GarethCabournDavies GarethCabournDavies left a comment

Choose a reason for hiding this comment

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

Minor changes, mostly to do with style/conventions. Functionally this looks like it does what I would expect

bin/pycbc_live Outdated Show resolved Hide resolved
pycbc/frame/frame.py Outdated Show resolved Hide resolved
pycbc/frame/frame.py Outdated Show resolved Hide resolved
dq_rate = numpy.maximum(dq_rate, 1)

logr_n = ExpFitFgBgNormStatistic.lognoiserate(
self, trigs)
logr_n += numpy.log(dq_rate)
return logr_n

def single(self, trigs):
# make sure every trig has a dq state
try:
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 an if hasattr(self, 'ifo') would be nicer here than the try/except. However I know that is against EAFP, and this is how it is done elsewhere

pycbc/frame/frame.py Outdated Show resolved Hide resolved
pycbc/frame/frame.py Show resolved Hide resolved
pycbc/events/stat.py Show resolved Hide resolved
pycbc/events/stat.py Show resolved Hide resolved
tnum = trigs.template_num
except AttributeError:
tnum = trigs['template_id']

try:
Copy link
Contributor

Choose a reason for hiding this comment

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

This try/except is used a couple of times now. It would be good to be in a function. Codeclimate will probably complain

pycbc/events/stat.py Outdated Show resolved Hide resolved
Copy link
Contributor

@GarethCabournDavies GarethCabournDavies 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 re: formatting, but I think this is good otherwise

bin/pycbc_live Outdated Show resolved Hide resolved
bin/pycbc_live Outdated Show resolved Hide resolved
bin/pycbc_live Outdated Show resolved Hide resolved
pycbc/events/stat.py Outdated Show resolved Hide resolved
pycbc/events/stat.py Outdated Show resolved Hide resolved
@titodalcanton
Copy link
Contributor

Please rebase on master to pick up #4855 and get the tests to pass.

@maxtrevor maxtrevor merged commit cf6447e into gwastro:master Aug 22, 2024
30 checks passed
@maxtrevor maxtrevor deleted the idq_live branch August 22, 2024 15:59
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