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

Accept negative m modes #5035

Closed
wants to merge 1 commit into from
Closed

Accept negative m modes #5035

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 26 additions & 14 deletions pycbc/waveform/waveform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,31 +396,43 @@ def parse_mode_array(input_params):
"""Ensures mode_array argument in a dictionary of input parameters is
a list of tuples of (l, m), where l and m are ints.

Accepted formats for the ``mode_array`` argument is a list of tuples of
ints (e.g., ``[(2, 2), (3, 3), (4, 4)]``), a space-separated string giving
the modes (e.g., ``22 33 44``), or an array of ints or floats (e.g.,
``[22., 33., 44.]``.
Accepted formats for the ``mode_array`` argument:
- List of tuples of ints: ``[(2, 2), (3, 3), (4, 4)]``
- Space-separated string: ``"22 33 44 2-2 4-4"``
- Array of ints or floats: ``[22., 33., 44., -22., -44.]``
"""
if 'mode_array' in input_params and input_params['mode_array'] is not None:
mode_array = input_params['mode_array']

# Convert string to list (splitting on spaces)
if isinstance(mode_array, str):
mode_array = mode_array.split()

# Ensure mode_array is iterable (list or numpy array)
if not isinstance(mode_array, (numpy.ndarray, list)):
mode_array = [mode_array]
for ii, ma in enumerate(mode_array):
# if ma is a float or int, convert to str (e.g., 22. -> '22'), so
# that...
if isinstance(ma, (float, int)):

parsed_modes = []
for ma in mode_array:
if isinstance(ma, (float, int)):
# Convert numeric form (e.g., 22 -> "22", -22 -> "2-2")
Copy link
Contributor

Choose a reason for hiding this comment

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

To handle the -22 case, you could just add the following in the if isinstance block:

if ma < 0:
    l, m = str(int(abs(ma))
    ma = f'{l}-{m}'
else:
    ma = str(int(ma))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I chose the inclination of pi/3 while making the above plot and I didn't see any difference and hence I think you are correct "2,2" really means "2,2 and 2,-2".

So, the fix is not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at this utility function (https://lscsoft.docs.ligo.org/lalsuite/lalsimulation/_l_a_l_sim_inspiral_waveform_flags_8c_source.html#l00481) ... or other functions in this module, which does some of the underpinning for the mode array, you can see that the negative m modes are separate from the positive m modes. So lalsimulation does support negative m modes. However, some waveforms, due to construction, cannot differentiate between (for e.g.) 2,2 and 2,-2, and so in those cases you can either have 2,2 and 2,-2, or neither. Phenom used to do this, I think it still does, but am not sure. The waveforms-from-NR-files functionality can handle positive and negative modes differently.

In cases such as these, I shouldn't be up to PyCBC to know what a given waveform will do, it should should pass data along. If lalsimulation does not provide a useful help/warning that you are asking for something that it cannot do, it's probably worth requesting a patch there.

ma = str(int(ma))
# if ma is a str convert to (int, int) (e.g., '22' -> (2, 2))

if isinstance(ma, str):
l, m = ma
ma = (int(l), int(m))
mode_array[ii] = ma
input_params['mode_array'] = mode_array
if '-' in ma:
# Handle cases like "2-2" -> (2, -2)
l, m = ma.split('-')
ma = (int(l), -int(m)) # Ensure m is negative
else:
# Handle cases like "22" -> (2,2)
l, m = ma[0], ma[1]
ma = (int(l), int(m))
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this? From what I can tell, this won't handle things like "-22" correctly. If the float -22 is passed in, line 419 will convert that to the string '-22'. But then line 424 will yield l = '' and m = '22' if ma is -22. Can you verify?

Copy link
Member

Choose a reason for hiding this comment

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

The time domain models I think all work OK, but if you recall it's frequency domain phenom models which behave a bit differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdcapano I tested with -22 for NRSur7dq4 and I got the error ValueError: invalid literal for int() with base 10: ''.
I can add this however:

            if isinstance(ma, str):
                if ma.startswith('-'):
                    raise ValueError(f"Invalid mode: {ma}. Modes cannot start with '-'.")

if you recommend it.

To answer @ahnitz I tried running with

[static_params]
approximant = IMRPhenomXPHM
f_ref = 40
f_lower = 40
tref = 1420878141.237796
ra = 2.570442706589624
dec = 0.5330886247974372
mode_array = 22 2-2 44 4-4

and the sampling is progressing:
178it [00:15, 10.94it/s, bound: 0 | nc: 1 | ncall: 1216 | eff(%): 14.638 | loglstar: -inf < -15200.530 < inf | logz: -15208.321 +/- nan | dlogz: 2896.283 > 0.100]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here are the attached time domain plots:
62e7201c-8ce1-4f2d-83dd-ab542d554dbb
91fd4cd9-7ab9-40a2-b19c-f6e940723ec2

Copy link
Contributor

Choose a reason for hiding this comment

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

In response to your question to me: you can support the -22 case as you have it in your doc string and comments. You just need to add some code to handle that case. See my suggestion above.

In regards to your plots: what inclination angle did you use? If you set it to zero, you'll get the same thing as 2,2 and 2,2+2,-2 because for inclination 0 the 2,-2 is zero. To see if either of these approximants are doing anything different when you give 2,-2 plot what you get for 2,2 only and what you get for 2,-2 only. They should be different. You could also try setting inclination to pi/2 and plot 2,2 and 2,2 + 2,-2. The waveforms should be different. If not, and you get the same thing always, then it means that "2,2" really means "2,2 + 2,-2"; i.e., that you cannot tell it to only generate the +m mode.


parsed_modes.append(ma)

input_params['mode_array'] = parsed_modes
return input_params


def props(obj, **kwargs):
""" Return a dictionary built from the combination of defaults, kwargs,
and the attributes of the given object.
Expand Down
Loading