-
Notifications
You must be signed in to change notification settings - Fork 346
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
Add any provided key to lal_pars
for waveform generation
#4728
base: master
Are you sure you want to change the base?
Conversation
cc @spxiwh. |
@spxiwh, I think I have fixed the test failures by only adding non-default flags to the |
Wait, what!? Why is there a |
for key, value in p.items(): | ||
if (value is None) or (key in default_args.keys()): | ||
continue | ||
camelcase = "".join([subkey.capitalize() for subkey in key.split("_")]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can any of the following lines be removed then if this is added? It would seem to serve in large part a similar purpose. Not all of course (modes for example would not work), but some of the nongr params probably could be not explicitly given now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahnitz, I would need to change the logic to account for the nongr
params, or potentially deprecate the existing pycbc
names and change them to new names. For example, the current nongr
params in pycbc
follow the convention e.g. dchi0
but the lalsimulation
function requires them to be NonGRDChi0
. I could remove frame_axis
and modes_choice
, but they would also need to be removed from the default_args
(in order to prevent the issue above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think we want to change the standardized names. One could remove code duplication with a dict, though, no?
Also, have you checked how slow this procedure is? What sort of overhead does it introduce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the LALDict internally uses exactly the same field name as pycbc anyway (https://git.ligo.org/lscsoft/lalsuite/-/blob/master/lalsimulation/lib/LALSimInspiralWaveformParams.c#L1326) - it is only the name of the 'Insert' function that is unpredictable.
After importing lalsimulation it is possible to get a mapping from the dictionary key to the Insert function in any case - LVK people can see here how to do that via dir(lalsimulation)
(it's not anything that should be secret ...)
https://git.ligo.org/waveforms/new-waveforms-interface/-/issues/49#note_1028009
FWIW, the redundant looking InsertMass1 (etc) functions are part of the 'New Waveform Interface', see here |
So the 'key' here is actually the last part of the SimInspiralInsert function name, rather than the LALDict key name itself, if I've understood correctly. I guess this is an inevitable consequence of the lalsim code not having a predictable mapping from the key name to the function name ... |
The purpose of this PR is to pass any non default flags provided to
pycbc
's waveform generation to the relevantlalsimulation
functions by adding them tolal_pars
. The code first checks to see if the provided key can be added, and then it tries a camelcase version. The keys are added via thelalsimulation.SimInspiralWaveformParamsInsert{key}
function. This code was inspired frombilby
: https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/gw/source.py?ref_type=heads#L583-586.I opted to try both e.g.
modes_choice
andModesChoice
as the prior is more convenient to add to configs/HDF. This change is particularly useful for modifying the default behaviour of waveform models (for example turning off multi-banding inIMRPhenomXPHM
). This change will affect all pipelines that provide additional flags to the waveform generator.