-
Notifications
You must be signed in to change notification settings - Fork 357
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
Accept negative m modes #5035
Accept negative m modes #5035
Conversation
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.
Thanks for doing this. I think there might be a bug in parsing '-22'; see my comment below. Also, are you certain that lalsimulation will handle -m modes? I think some waveform approximants will always include the -m modes, and so it only will take +m modes. Not sure about that though; I haven't looked at this in a few years. If you can confirm by generating a waveform with 2,-2 and 2,2 and check that you get different results, that would be a good test to see.
else: | ||
# Handle cases like "22" -> (2,2) | ||
l, m = ma[0], ma[1] | ||
ma = (int(l), int(m)) |
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.
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?
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.
The time domain models I think all work OK, but if you recall it's frequency domain phenom models which behave a bit differently.
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.
@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]
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.
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.
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 = [] | ||
for ma in mode_array: | ||
if isinstance(ma, (float, int)): | ||
# Convert numeric form (e.g., 22 -> "22", -22 -> "2-2") |
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.
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))
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.
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.
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.
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.
else: | ||
# Handle cases like "22" -> (2,2) | ||
l, m = ma[0], ma[1] | ||
ma = (int(l), int(m)) |
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.
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.
Fix: Properly parse mode_array to support negative m values
parse_mode_array
to correctly handle mode inputs with negative m values (e.g., "2-2" → (2, -2))22.
→(2,2)
,-22.
→(2,-2)
)"22 2-2 44 4-4"