-
Notifications
You must be signed in to change notification settings - Fork 371
Quadtratic modes #5198
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
base: master
Are you sure you want to change the base?
Quadtratic modes #5198
Conversation
cdcapano
left a comment
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.
Hi @nishkalrao20, thanks for taking this on!
It looks like you have some unintended changes committed here. Why the changes to the examples workflows? Also, you made a change to single.ini in the inference/small_test, which I don't think should be a part of this PR. Please remove those commits.
That said, it would be good for you to add an example configuration to show how to set up one of these runs.
I have a few other comments. There is a lot here, so I'm also adding Yifan to take a look, since he's done similar things.
| # are three digits long, and that nmodes!=0 | ||
| for qlmn in qlmns: | ||
| # Quadratic modes are given as qlmn=lmn1xlmn2 | ||
| qlmn, lmn12 = qlmn.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.
I wouldn't use an = sign here. That could cause issues. How about using a colon instead; i.e., qlmn:lmn1xlmn2?
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.
I agree. So far, it hasn't caused issues in the runs, but I agree that it is better if we shift to a colon.
| qlmns = qlmns.split(' ') | ||
| # Case 2: the qlmns are given as strings in a list, e.g. ['221', '331'] | ||
| elif isinstance(qlmns, list): | ||
| pass |
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.
Why an elif that leads to a pass? It'd make more sense to just add a second if that leads to the ValueError. In other words:
| pass | |
| if not isisntance(qlmns, list): | |
| raise ValueError(...) |
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.
Yeah, I agree. I realized a similar syntax has been used in format_lmns too. Do I change that too?
| # reference mode | ||
| ref_amp = kwargs.pop('ref_amp', None) | ||
| if ref_amp is None: | ||
| # default to the 220 mode | ||
| ref_amp = 'amp220' | ||
| # check for reference dphi and dbeta | ||
| ref_dbeta = kwargs.pop('dbeta', 0.) | ||
| ref_dphi = kwargs.pop('dphi', 0.) | ||
| if isinstance(ref_amp, str) and ref_amp.startswith('amp'): | ||
| # assume a mode was provided; check if the mode exists | ||
| ref_mode = ref_amp.replace('amp', '') | ||
| try: | ||
| ref_amp = kwargs.pop(ref_amp) | ||
| amps[ref_mode] = ref_amp | ||
| except KeyError: | ||
| raise ValueError("Must provide an amplitude for the reference " | ||
| "mode {}".format(ref_amp)) |
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.
This block is copied from lm_amps_phases. Copying code is usually a bad idea. I'd move this to a stand-alone function (maybe, get_ref_mode_params), then have both lm_amps_phases and qlm_amps_phases call it.
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.
I agree. I couldn't help but copy the base functions, but unifying them as a bigger function is helpful.
| raise ValueError('Must provide reference phi for quadratic modes, and \ | ||
| phi{} and phi{} are required'.format(mode1, mode2)) | ||
| dphis[modeq] = kwargs.pop('dphi'+qlmn, ref_dphi) | ||
| dbetas[modeq] = kwargs.pop('dbeta'+qlmn, ref_dbeta) |
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.
Does it make sense to have a dphi and dbeta for the quadratic modes? These parameters allow the phases and amplitudes of the -m modes to differ from the +m modes, as you would expect if equatorial symmetry is broken. Would you also expect that freedom for the quadratic modes, or would that be sourced from linear modes?
|
|
||
| def format_qlmns(qlmns): | ||
| """Checks if the format of the parameter qlmns is correct, returning the | ||
| appropriate format if not, and raise an error if nmodes=0. |
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.
Here and in other doc strings, please initially define the qlmns as the quadratic modes. It took me awhile to realize what qlmn stood for.
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 am sorry. I will fix the same.
| as a single whitespace-separated string, with n the number | ||
| of overtones desired. Alternatively, a list object may be used, | ||
| containing individual strings as elements for the qlmn modes. | ||
| For instance, qlmns = '221 331' are the modes 220, 221, 222, and 330. |
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.
Wouldn't 221 331 give 220 330? I think for 220, 221, 222, you'd need to specify 223.
I regret that we made the N be the number of modes rather than the overtone number. It's made it confusing on how to specify overtones. At some point I would like to switch to explicitly setting overtones (i.e., instead of setting lmns = 222 for 220+221, you would do lmns = 220 221). However, that will break backward compatibility, so would require some thought to implement. This PR isn't the place for that.
That said, does it make sense to follow the same syntax for quadratic modes? It might add to the confusion, but maybe for the qlmns the n should be the overtone number rather than the number of modes. The issue I could see with the current format is you wouldn't be able to form a quadratic mode from different overtones, although I don't know if that's possible or not.
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.
Sorry, the doc string was copied from the lmn mode. I will fix the same.
Yeah, I agree that
I agree. In the present syntax, it is wrong if we want a quadratic mode from different overtones, because we specify: 441=221x221 442=222x222, I believe it messes up since 222 is 220+221. So far, for the tests, I have used only 221. Any suggestions on how to generate quadratic modes from different overtones? It would be helpful if the lmn syntax is changed.
|
|
||
| def get_qlm_f0tau_allmodes(mass, spin, qmodes): | ||
| """Returns a dictionary of all of the frequencies and damping times for the | ||
| requested modes. |
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.
Again, make explicit what the q stands for in the doc string.
| The modes to get. Each string in the list should be formatted | ||
| 'qlmN', where l (m) is the l (m) index of the | ||
| harmonic and N is the number of overtones to generate (note, N is not | ||
| the index of the overtone). |
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.
As I said above, I think it more sense with the quadratic lmns to explicitly specify the overtone number, rather than make N the number of modes.
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.
I agree, as I pointed out there. Should I just change the quadratic mode overtone number? So if we had 442=222x222, I would have to change the code such that 222 corresponds to 221 in the new syntax, and not 220+221.
| # skip | ||
| continue | ||
| if domain == 'td': | ||
| print('{}: Frequency: {}, Tau: {}'.format(lmn, freqs[lmn], taus[lmn])) |
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.
Debugging print statements should be removed.
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.
I am really sorry. This change had been removed locally, I somehow am unsure how this got pushed.
Implementation of Quadratic modes in Ringdown
Standard information about the request
This is a: new feature
This change affects: inference
This change changes: scientific output
This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines
This change will: affect
waveform/ringdown.py, with major additions to themultimode_basefunction. Supplemental edits to theconversions.py.Motivation
In general relativity, perturbed black holes formed in binary mergers settle to a stationary state by emitting gravitational waves (GWs) through a discrete set of quasi-normal modes (QNMs). While the linear QNMs have been extensively studied, recent numerical simulations and theoretical investigations (References: 1, 2, 3) suggest that nonlinear (quadratic) modes may also be excited during the merger process. Implemented and tested quadratic mode injections and analyzed their detectability.
Beyond the linear approximation, the nonlinear nature of Einstein's equations where first-order perturbations will act as effective sources for second-order (quadratic) modes. At this level, the total waveform can be expressed as a sum of linear and quadratic contributions. Schematically, we have$$h_{+}-ih_{\times}=\frac{M}{r}\sum_{k\equiv\ell m n}A_{k}\exp\bigl[i\omega_{k}t+\Phi_{k}\bigr] Y_{k}$$ where the index $k$ runs over both linear modes and their nonlinear (quadratic) counterparts.
For quadratic modes generated by the coupling of two linear modes denoted$k_1\equiv(\ell_1 m_1 n_1)$ and $k_2\equiv(\ell_2 m_2 n_2)$ , the following relations are expected:
$\omega_{\ell m n} = \omega_{\ell_1 m_1 n_1}+\omega_{\ell_2 m_2 n_2}$
$\frac{1}{\tau_{\ell m n}} = \frac{1}{\tau_{\ell_1 m_1 n_1}}+\frac{1}{\tau_{\ell_2 m_2 n_2}}$
$A_{\ell m n} \propto A_{\ell_1 m_1 n_1}A_{\ell_2 m_2 n_2}$
$\Phi_{\ell m n} = \Phi_{\ell_1 m_1 n_1}+\Phi_{\ell_2 m_2 n_2}+\text{const.}$
Implementation of the amplitudes and phase is done through the couplings prescribed, with an extra parameter for the coefficient of proportionality. The frequencies and damping times serve the agnostic runs, through the above relations.
Contents
Syntax for the quadratic modes required is
qlmns=lmn=l1m1n1 x l2m2n2 (e.g, 440=220x220).An example of the relevant snippet of the configuration file:
Links to any issues or associated PRs
Testing performed
The code has been rigorously tested in a companion repository: quadratic_bbh with injections, PE runs, and verifying the detectability.
Additional notes