Add a feature to fit noise trigger rates with respect to eccentricity - #5328
Add a feature to fit noise trigger rates with respect to eccentricity#5328yi-fan-wang wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for fitting/smoothing background (noise trigger rate) fit coefficients as a function of additional template parameters needed for eccentric searches, extending the existing template-parameter regression infrastructure used by the offline all-sky search tooling.
Changes:
- Extend
pycbc.events.triggers.get_paramto support derived mass-ratioqand bank-storedeccentricity. - Update all-sky background-fit utilities to allow
q/eccentricityas smoothing dimensions and to use the updatedget_paramcalling pattern. - Add a guard in
pycbc_fit_sngls_over_multiparamto error out if smoothed counts above threshold are non-positive (prevents downstreamlog(rate)failures).
Reviewed changes
Copilot reviewed 1 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pycbc/events/triggers.py |
Changes get_param interface and adds q/eccentricity parameter support. |
bin/all_sky_search/pycbc_fit_sngls_over_multiparam |
Enables smoothing over q/eccentricity, updates parameter extraction, and validates smoothed counts. |
bin/all_sky_search/pycbc_fit_sngls_by_template |
Updates pruning parameter extraction to use the revised get_param calling convention. |
bin/all_sky_search/pycbc_fit_sngls_binned |
Updates pruning/binning parameter extraction helper to use the revised get_param calling convention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parvals = conversions.q_from_mass1_mass2(m1, m2) | ||
| elif par in ['chi_eff', 'effective_spin']: | ||
| parvals = conversions.chi_eff(m1, m2, s1z, s2z) | ||
| elif par == 'eccentricity': |
| elif par == 'q': | ||
| parvals = conversions.q_from_mass1_mass2(m1, m2) | ||
| elif par in ['chi_eff', 'effective_spin']: | ||
| parvals = conversions.chi_eff(m1, m2, s1z, s2z) | ||
| elif par == 'eccentricity': | ||
| parvals = bank['eccentricity'][:][tid] |
| def get_param(par, args, bank, tid): | ||
| """ | ||
| Helper function | ||
| Helper function to extract parameters from bank and calculate | ||
| derived parameters | ||
|
|
||
| Parameters | ||
| ---------- | ||
| par : string | ||
| Name of parameter to calculate | ||
| args : Namespace object returned from ArgumentParser instance | ||
| Calling code command line options, used for f_lower value | ||
| m1 : float or array of floats | ||
| First binary component mass (etc.) | ||
| bank : h5py File object | ||
| Bank parameter file | ||
| tid : integer or array of int | ||
| Indices of the entries to be returned | ||
|
|
||
| Returns | ||
| ------- | ||
| parvals : float or array of floats | ||
| Calculated parameter values | ||
| """ | ||
| m1, m2, s1z, s2z = get_mass_spin(bank, tid) |
|
Apparently, more functions rely on |
| **kwarg_dict | ||
| ) | ||
|
|
||
| non_positive_mask = smoothed_vals[:, 0] <= 0 |
There was a problem hiding this comment.
This extra code, and specifically the fact that it raises an error doesn't seem to be within the scope of the new feature. The code was designed to be robust to a small number of zero-trigger templates, of course it fails later in the averaging step if a significant / large fraction of templates have no triggers.
There was a problem hiding this comment.
Hi @tdent thanks for the comment! I think you point is that it's fine to have zero triggers for some templates before averaging/smoothing, but this is already the smoothed values after being averaged/smoothed. My motivation to introduce this line is exactly, as you said, to let it fail in the averaging step if a significant / large fraction of templates have no triggers. Otherwise it won't fail and hence cause a problem, as far as I'm aware of. Let me know if I miss anything or misunderstood.
There was a problem hiding this comment.
Ah, right, if this is after the smoothing step it makes sense. However, the description of the changes / extra feature doesn't mention that you add this, and it's logically not connected to adding eccentricity. Can you split it off into a separate PR?
This PR was used in the work https://arxiv.org/abs/2508.05018 for a search with eccentricity. I added a feature to fit the noise trigger rate as a function of eccentricity and smooth over it. Mass ratio
qis also added.Standard information about the request
This is a new feature
This change doesn't affect previous results.
This change changes scientific output
This change follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines. But it doesn't have unittest yet.
This change will not break any current functionality, it should have backwards compatibility to any previous noise trigger rate fit and smoothing in offline search.
Motivation
Add this for an offline search including eccentricity
Contents
It changes pycbc/events/triggers.py and three other pycbc_fit_* related commands in bin, see code diff for more details.
Links to any issues or associated PRs
Testing performed
I don't have any explicit tests at the moment but try to add some unittests in the future. I can point to the configuration file of my work https://arxiv.org/abs/2508.05018, where we use this PR and thus it's some tests
Additional notes