-
Notifications
You must be signed in to change notification settings - Fork 213
Add translation to transition to ringdown script #6808
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: develop
Are you sure you want to change the base?
Add translation to transition to ringdown script #6808
Conversation
cb37705 to
3a24d16
Compare
3a24d16 to
0b89dc2
Compare
eb5ca91 to
aa50963
Compare
aa50963 to
d413b76
Compare
markscheel
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.
I may have missed it, but I don't see comments corresponding to lines 1209-1237 of SpEC's ConstructPostMergerDataFromAhC.pl, where the translation map is corrected to account for translation/rotation in the inspiral. This correction is easy to overlook; are you doing the correction?
| } // namespace | ||
|
|
||
| void bind_strahlkorper_transformations(py::module& m) { // NOLINT | ||
| // Only instantiating for Grid->Inertial because the Py functions are |
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.
Update dox here?
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 binding was not needed and left in by mistake
| false, | ||
| std::nullopt, | ||
| {100.0}, | ||
| {50.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.
Add comments as to why 50 and not 100 (or some other number). Same for other magic numbers here.
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 don't understand what "Radial partition used in current ringdowns" means or why it is 50.
| if "Translation" not in evaluated_fot_dict: | ||
| evaluated_fot_dict["Translation"] = None |
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 is this needed?
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 added this so that we don't try to read in the history of the translation map from the inspiral if there wasn't one used.
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.
Ok, pls add comment.
| /*! | ||
| * \brief Transform `Strahlkorper` coefs to ringdown distorted frame. | ||
| * \brief Transforms `Strahlkorper` coefs to ringdown distorted frame and tracks | ||
| * the center of mass. |
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 track the center of mass, or does it track the geometric center of AhC? I think the latter.
Maybe Reads inertial-frame Strahlkorper coefs from a file, returns the ringdown-distorted-frame coefs and the Strahlkorper's inertial-frame geometric center ?
| * ringdown distorted frame defined by the expansion, rotation, and translation | ||
| * maps specified by `exp_func_and_2_derivs`, | ||
| * `exp_outer_bdry_func_and_2_derivs`, `rot_func_and_2_derivs`, and | ||
| * `trans_func_and_2_derivs`. The expansion and rotation functions of time | ||
| * correspond to the ringdown frame's expansion and rotation maps at the time | ||
| * given by `match_time`, and by `settling_timescale`, the timescale for the | ||
| * maps to settle to constant values. The translation function of time supplied | ||
| * does not correspond to the ringdown frame's translation function of time. The | ||
| * ringdown's translation function of time needs to be built by tracking the | ||
| * position of the center of mass at multiple times. Only Strahlkorpers and | ||
| * center of mass points within `requested_number_of_times_from_end` times from | ||
| * the final time are returned. This function is used to transition from | ||
| * inspiral to ringdown; in this case, the inertial-frame Strahlkorper is the | ||
| * common apparent horizon from a binary-black-hole inspiral; the | ||
| * ringdown-distorted-frame coefficients are used to initialize the shape map | ||
| * for the ringdown domain. The center of mass points are used to initialize the | ||
| * translation map for the ringdown domain. |
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.
What is the translation function of time that is passed into this function? Is it the translation function of time of the inspiral? (You say what it is not, but not what it is).
And what are "center of mass points"? Does the center of mass actually matter here?
Also, should say that the AhC center is returned in the inertial frame.
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.
Yes, the translation function of time is from the inspiral. And no, the center of mass doesn't actually matter here, I originally assumed that the center of mass and geometric center of the strahlkorper were the same.
| tnsr::I<DataVector, 3, ::Frame::Inertial> inertial_center_point{ | ||
| DataVector{3, 0.0}}; | ||
| // The center point is mapped back to the inspiral inertial frame so that | ||
| // the center of mass is the same at the match time. |
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.
center of mass -> center of AhC
| */ | ||
| std::vector<DataVector> strahlkorper_coefs_in_ringdown_distorted_frame( | ||
| std::pair<std::vector<DataVector>, std::vector<std::array<double, 3>>> | ||
| strahlkorper_coefs_in_ringdown_distorted_frame( |
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.
Maybe rename? The function name sounds like it is returning only the coefs.
| expansion_map = ExpansionMapOptions([1.0, 1e-4, 0.0], 100.0, 1e-6) | ||
| rotation_map = RotationMapOptions([[0.0, 0.0, 0.0, 1.0]], 100.0) | ||
| translation_map = TranslationMapOptions( | ||
| [[1.0, -1.0, 0.5], [0.0, 0.0, 0.0], [0.0, 0.0, 0.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.
Do you want to test with nonzero velocity?
src/Evolution/Ringdown/StrahlkorperCoefsInRingdownDistortedFrame.cpp
Outdated
Show resolved
Hide resolved
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.
Yes I believe this correction is being applied to account for the inspirals rotation, expansion, and translation. This is done by transforming the geometric center from the ringdown-distorted-frame EDIT: back to the inspiral-inertial-frame to the ringdown-inertial-frame. Those center points are then used to fit to a cubic polynomial. I added some comments in the details, but let me know if I missed anything or if more should be added :)
| make_not_null(&distorted_ahc), 1e-7); | ||
| ahc_ringdown_distorted_coefs.push_back(distorted_ahc.coefficients()); | ||
|
|
||
| tnsr::I<DataVector, 3, ::Frame::Grid> grid_center_point{ |
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.
Wanted to point out that I changed these lines in this fixup because doing tnsr::I<DataVector, 3, ::Frame::Grid> {distorted_ahc.expansion_center()} was not constructing the tensor correctly. It was producing a tensor with a DataVector with 3 component at each index which is not what I was trying to do there.
bab84f6 to
1978a2d
Compare
| // the center of AhC is the same at the match time. | ||
| coords_to_different_frame( | ||
| make_not_null(&inertial_center_point), grid_center_point, | ||
| ringdown_domain, ringdown_functions_of_time, gsl::at(ahc_times, i)); |
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 want to highlight this change, that was causing the center to be placed incorrectly. I misread what the SpEC script was doing and thought it was taking this center point back to the inspiral inertial frame, but it actually maps it to the ringdown inertial frame instead. Doing this placed the excision center correctly for the 2 mass ratio ringdown as can be seen below.

|
@markscheel and @nilsvu I've pushed a fixup adding some more comments, so I think this is ready for another round of reviews :) |
6ed71b3 to
e9ed405
Compare
markscheel
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.
In SpEC's ConstructPostMergerDataFromAhC, there's a 28-line detailed comment, including matrix algebra, that explains why all of this works even though translation and rotation don't commute and the center of AhC is not the origin. In particular, it explains why you can use the coefficients of AhC as-is except for a minus sign, and how you compute the coefficients of the translation map. Please add a comment that is at least as detailed as the SpEC comment.
There is also a 32-line detailed comment in ConstructPostMergerFromAhC explaining what the 'ringdown distorted frame' is (in SpEC this is sometimes called the 'intermediate frame'), and how it is constructed, and explains details like "The ringdown translation map is different than the plunge one because it is tracking a different center, so translation is discontinuous in time". I think all of that still applies here (but let me know if I am wrong). Please add a comment that is at least as detailed as the SpEC comment.
| translation function of time for the ringdown using the functions of time | ||
| and the AhC coefficients in the Inspiral inertial frame. | ||
| Arugments: |
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.
Minor: spelling.
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.
ping on this
| ( | ||
| fit_ahc_translation_coefs, | ||
| fit_ahc_dt_translation_coefs, | ||
| fit_ahc_dt2_translation_coefs, | ||
| ) = fit_to_a_cubic( | ||
| ahc_times_for_fit, | ||
| ahc_inertial_centers_for_fit, | ||
| match_time, | ||
| zero_coefs_eps, | ||
| ) | ||
| ahc_translation_fot = [ | ||
| fit_ahc_translation_coefs, | ||
| fit_ahc_dt_translation_coefs, | ||
| fit_ahc_dt2_translation_coefs, | ||
| ] |
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.
Just asking: is there a reason this can't be
ahc_translation_fot = fit_to_a_cubic(...)
instead of doing it in two steps?
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.
no reason, I'll swap it to that
| translation_map_options, | ||
| translation_fot_from_volume, | ||
| true}; | ||
| const domain::creators::Sphere domain_creator{ |
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 appears to make a fake Domain that is different from the actual Domain that will be used for the final ringdown. Below you used to call it temporary_domain, so I think my interpretation is correct, but now it's no longer called temporary_domain (why)? Please add a (long, at least one paragraph, probably several paragraphs) comment of why you are doing this in the first place, what will this fake domain be used for, and what assumptions are being made about the fake domain (e.g. why is the inner radius so tiny as to make the domain extremely distorted [I don't think SpEC does this]? Why is the outer radius only 200M? Do you assume that AhC in the ringdown must have r < 200M [and if so, in what frame?] Why only 5 grid points?).
| false, | ||
| std::nullopt, | ||
| {100.0}, | ||
| {50.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.
I don't understand what "Radial partition used in current ringdowns" means or why it is 50.
| translation function of time for the ringdown using the functions of time | ||
| and the AhC coefficients in the Inspiral inertial frame. | ||
| Arugments: |
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.
ping on this
| None, | ||
| ) | ||
| shape_and_translation_coefs = Ringdown.strahlkorper_coefs_and_centers( | ||
| str(path_to_volume_data), |
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.
Use named arguments. Makes it clearer what each argument means and prevents bugs.
632058b to
0d76c3a
Compare


Proposed changes
First pr of 3 adding the rest of the transition to ringdown script from SpEC to SpECTRE.
This pr adds the tracking of the geometric center of the common horizon which will then be used to make the translation function of time used in the ringdown. This is necessary for unequal mass simulations to fully ringdown.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments