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.

Sign up for GitHub

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

[BUG] Fix passing mode_array in injection-waveform-arguments #820

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[BUG] Fix passing mode_array in injection-waveform-arguments #820

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c550ae8
Fix passing mode_array in injection-waveform-arguments
lorenzo-pompili Oct 6, 2024
4d85792
Add name to authors.md
lorenzo-pompili Oct 6, 2024
f3652b6
Put conversion inside if statement
lorenzo-pompili Oct 7, 2024
d0ced8d
Add try/except
lorenzo-pompili Oct 7, 2024
ba16abe
Typo
lorenzo-pompili Oct 7, 2024
29c325c
Merge branch 'main' into fix_injection_mode_array
ColmTalbot Nov 14, 2024
8d93f18
Add more informative error message
lorenzo-pompili Jan 16, 2025
97f82cc
Add safe_cast_mode_to_int
lorenzo-pompili Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,5 @@ Martin White
Peter Tsun-Ho Pang
Alexandre Sebastien Goettel
Ann-Kristin Malz
Lorenzo Pompili
Sean Hibbitt
9 changes: 8 additions & 1 deletion bilby/gw/source.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,13 @@ def gwsignal_binary_black_hole(frequency_array, mass_1, mass_2, luminosity_dista
}

if mode_array is not None:
try:
mode_array = [tuple(map(int, mode)) for mode in mode_array]
except (ValueError, TypeError) as e:
raise ValueError(
f"Unable to convert mode_array elements to tuples of ints. "
f"mode_array: {mode_array}, Error: {e}"
) from e
gwsignal_dict.update(ModeArray=mode_array)

# Pass extra waveform arguments to gwsignal
Expand Down Expand Up @@ -528,7 +535,7 @@ def set_waveform_dictionary(waveform_kwargs, lambda_1=0, lambda_2=0):
if mode_array is not None:
mode_array_lal = lalsim.SimInspiralCreateModeArray()
for mode in mode_array:
lalsim.SimInspiralModeArrayActivateMode(mode_array_lal, mode[0], mode[1])
lalsim.SimInspiralModeArrayActivateMode(mode_array_lal, int(mode[0]), int(mode[1]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the motivation for wrapping mode[0] inside an int? I have the following concern in mind: say mode[0] in reality is zero, but due to some numerical issues it gets set to 1.999999 (or something like that). This would make int(mode[0]) = 1, which is totally different.

In any case, I suggest casting to int (taking consistency checks like the above into account) when the mode array is set.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lalsimulation is pretty strongly typed, the arguments have to be forced into int. We could be stricter, but sometimes Python will convert ints to float by association with other floats.

In [70]: lalsim.SimInspiralModeArrayActivateMode(mode_array, 2.0, 2.0)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[70], line 1
----> 1 lalsim.SimInspiralModeArrayActivateMode(mode_array, 2.0, 2.0)

TypeError: in method 'SimInspiralModeArrayActivateMode', argument 2 of type 'unsigned int'

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is done differently for the gwsignal case

mode_array = [tuple(map(int, mode)) for mode in mode_array]

I'm not too worried by the different implementations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Follow-up question; why do we need this now when we didn't before?

Copy link
Author

Choose a reason for hiding this comment

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

As far as I can tell this has always been an issue, the use-case in which one gets this error (requesting different mode arrays for the injection and recovery waveform via bilby_pipe) is just not very frequent, I suppose.

IMO the main issue is in bilby_pipe, which does not parse something like

injection-waveform-arguments={'mode_array':[[2,2],[2,-2]]}

as a list of lists in ints, but as a list of lists of strings, which ends up triggering the error above (see also https://git.ligo.org/lscsoft/bilby_pipe/-/issues/293). However there are some unit tests there which break if this behavior is changed (see https://git.ligo.org/lscsoft/bilby_pipe/-/merge_requests/585), which is the motivation for wrapping mode[0], mode[1] to int directly in bilby.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given @adivijaykumar's concern, would it be safer to have a function that only maps allowed types to int? I was thinking of something like this, which could then be called using map.

def safe_cast_to_int(value):
    """Converts a string or integer to an integer.

    Raises an error if the value is a float or any unsupported type.

    Parameters
    ---------------
    value:
         The input value to be cast to an integer.

    Returns
    ----------
    int: 
        The converted integer.

    Raises
    ---------
    TypeError
         If the input is a float or an unsupported type.
    ValueError
        If the string cannot be converted to an integer.
    """
    if isinstance(value, int):
        return value
    elif isinstance(value, str):
        try:
            return int(value)
        except ValueError:
            raise ValueError(f"Cannot convert string '{value}' to an integer.")
    elif isinstance(value, float):
        raise TypeError("Conversion from float to int is not allowed.")
    else:
        raise TypeError(f"Unsupported type '{type(value).__name__}'.")

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, I am happy do add this. What do you think would be a good place for this? Maybe https://github.com/bilby-dev/bilby/blob/main/bilby/core/utils/conversion.py?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I'd consider renaming it to be more specific (something like safe_cast_mode_to_int or similar) and putting it in bilby.gw.utils. I think conversions is more for conversions between parameters (I also think bilby.core.utils.conversion might get moved/deprecated at some point, since they're GW functions in core).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I added the function you suggested together with some tests

lalsim.SimInspiralWaveformParamsInsertModeArray(waveform_dictionary, mode_array_lal)
return waveform_dictionary

Expand Down
Loading