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

[lorenzopompili00]

When doing injections with bilby_pipe, passing a mode_array for the injected waveform via injection-waveform-arguments does not currently work, because of a missing conversion from string to int. See issue for details. Doing this conversion in bilby_pipe breaks some unit tests (see MR), so I have implemented the conversion in bilby only when mode_array is called.

Note that pyseobnr currently wants mode_array to be a list of tuples, but I don't think a standard convention has been decided for future models in gwsignal.

I have tested this by adding

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

to the example bbh_injection.ini, as well as for

injection-waveform-arguments={'mode_array':[[2,2],[2,1]]}
injection-waveform-approximant="SEOBNRv5PHM"
injection-frequency-domain-source-model=gwsignal_binary_black_hole

(I also had to include the fix for #41).

[ColmTalbot]

I think we should wrap these changes in a try/except like here so that we get an informative error if/when this fails for any reason. We can also have a more specific message here as it is a more specific case.

[mj-will]

Looks good to me, just had one comment about making the error message more informative.

[mj-will]

LGTM, assuming the CI passes.

[adivijaykumar]

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.

[ColmTalbot]

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'

[ColmTalbot]

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.

[mj-will]

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

[lorenzopompili00]

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.

[mj-will]

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__}'.")

[lorenzopompili00]

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?

[mj-will]

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).

[lorenzopompili00]

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

[ColmTalbot]

I'm happy with this as is.

[ColmTalbot]

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.