search

bilby-dev / bilby

A unified framework for stochastic sampling packages and gravitational-wave inference in Python. Note that we are currently transitioning from git.ligo.org/lscsoft/bilby, please bear with us!
https://bilby-dev.github.io/bilby/
MIT License
59 stars 66 forks source link

FEATURE: Warn users when waveform_kwargs are being overwritten or set to defaults #739

Open bilby-bot opened 1 year ago

bilby-bot commented 1 year ago

In GitLab by @git.ligo:jacob.golomb on Mar 30, 2023, 19:30

When setting the waveform_kwargs for the waveform_generator it is easy to make seemingly harmless mistakes like setting waveform_arguments['approximant'] = approximant rather than waveform_arguments['waveform_approximant'] = approximant, for example (speaking from experience). This means that the user's intended setting for the approximant to use is silently set to the default "IMRPhenomPv2" in this line for lal_binary_black_hole (different source models have different default kwargs). I think this is an easy enough mistake to make that bilby should log a warning when updating waveform_kwargs to default values so that users can track down where their intended settings do not match what bilby is assuming under the hood.

I think such a warning cannot be put where the kwargs are updated here, because that would mean it is printed every time the source model is called by the sampler. There also does not seem to be a straightforward place to put this in the waveform_generator's init, since the waveform_generator object cannot access the default waveform_kwargs which is stored as a local variable inside the source model. My proposal is to organize source.py a bit better by making the default waveform_kwargs no longer local variables defined inside the function (i.e. here), but make a dict default_kwargs with a map for each source model, which would be stored outside the function. For example, the line linked above would be accessed with default_kwargs['lal_binary_black_hole']. Also this means the waveform_generator could access this dict and determine what bilby will be adding to the user-specified waveform_kwargs. (i.e. f"{key} not specified in waveform_kwargs, setting to {default_kwargs[frequency_domain_model][key]}").

If this seems like a sensible proposal I can make an MR. I don't see it causing any backwards-incompatible changes.

bilby-bot commented 1 year ago

In GitLab by @git.ligo:colm.talbot on Apr 11, 2023, 14:57

I think this is a good idea in principle. The first caveat that comes to mind is that we need to make sure this doesn't fail when people try to use a custom source model, but this is simple to resolve by using a default_kwargs.get query.

bilby-bot commented 11 months ago

In GitLab by @git.ligo:colm.talbot on Nov 13, 2023, 03:52

Is !1269 a suitable fix to this issue?

bilby-bot commented 10 months ago

In GitLab by @git.ligo:jacob.golomb on Dec 27, 2023, 00:50

Yeah, I think this issue can be closed when that is merged. It looks like the behavior will be to pop all the required arguments from the waveform arguments dictionary and raise an error if there is anything remaining, which will prevent this issue from happening.

bilby-bot commented 2 months ago

In GitLab by @git.ligo:michael.williams on Aug 30, 2024, 12:23

That has been merged, so I think is partially addressed. (For now, a warning is raised instead of an error). @git.ligo:jacob.golomb I'll let you decide if you want to close this now or waiting for a change that raises an error (this will be a future release).