Find apparent horizon in distorted frame #4720

markscheel · 2023-02-10T15:41:14Z

Proposed changes

Two main changes:

compute_dest_vars_from_src_vars now recognizes distorted frame, so interpolation can be done in that frame.
Test_ApparentHorizonFinder now has a test where it finds the horizon in the distorted frame.

Other necessary changes are fixing a bug in UniformTranslation and adding instantiations of the distorted frame in many places.

Code review checklist

The code is documented and the documentation renders correctly. Run
make doc to generate the documentation locally into BUILD_DIR/docs/html.
Then open index.html.
The code follows the stylistic and code quality guidelines listed in the
code review guide.
The PR lists upgrade instructions and is labeled bugfix or
new feature if appropriate.

knelli2

You can squash directly

knelli2 · 2023-02-11T02:18:50Z

src/ParallelAlgorithms/Interpolation/InterpolationTargetDetail.hpp

+                    functions_of_time),
+                InterpolationTarget_detail::evaluate_temporal_id_for_expiration(


Why is this not InterpolationTarget_detail::get_temporal_id_value? I know it's used above, but looking at the two functions, they are identical for all types except TimeStepId. The difference is that get_temporal_id does TimeStepId.substep_time().value() while evaluate_temporal_id_for_expiration does TimeStepId.step_time().value(). They are different, but does that matter here?

That's a good question. evaluate_temporal_id_for_expiration is used in the code for the other frames too, and I thought that was what @wthrowe wanted. @wthrowe should I be using get_temporal_id_value here (and for the other frames) instead?

Probably _value, but first, do you get any failures if you put ASSERT(time_id.substep() == 0, "Interpolating on substep " << time_id); in both get_temporal_id_* functions?

@wthrowe all the tests pass if I put those ASSERTs into both functions. Should evaluate_temporal_id_for_expiration be removed, and only get_temporal_id_value used everywhere? (and should I keep the ASSERT?)

So at the moment, the functions always return the same value. I'm not sure I want to guarantee that that will always be true, though. If I remember right, this is only used during self start, which is only used for AdamsBashforth which doesn't have substeps. Predictor-corrector methods will presumably go through a similar infrastructure with substeps, though.

I don't really remember what's going on here. Why are there two of these? The _value function is presumably to represent the time (or iteration or whatever) to use in maps and such. What is the _expiration for? My first thought was we needed something monotonic, but it isn't monotonic. (Can these things get some documentation?)

Both of these were added in #3421. It seems that evaluate_temporal_id_for_expiration was added because previously, VerifyTemporalIdsAndSendPoints.hpp used pending_id.step_time().value(). Whereas, get_temporal_id_value was added because ObserveTimeSeriesOnSurface.hpp used temporal_id.substep_time().value(). (take a look at the first commit of that PR)

So we needed one that got the step_time and the other that got the substep_time. However, I'm not sure if we need to use one or the other in each circumstance. I agree with @wthrowe that _value seems to just get the current time, but unclear what _expiration is actually needed for.

Looking more closely, I don't think any of the uses of step_time() here are correct. (Uses of step_time() outside time-related code are usually wrong.) So just get rid of that function and probably rename the other one to get rid of the now-unnecessary disambiguator.

Ok, I will keep the one using substep_time() and remove the other one. Should I leave the ASSERT in there?

I say yes. It's just an ASSERT so won't affect productions stuff

No. I intend to add valid cases where it will fail.

src/ParallelAlgorithms/Interpolation/InterpolationTargetDetail.hpp

markscheel · 2023-02-17T16:44:33Z

squashed.

nilsdeppe · 2023-02-21T17:27:26Z

@wthrowe did you want to look at the part you were involved in discussing for this PR?

wthrowe · 2023-02-21T19:06:26Z

src/ParallelAlgorithms/Interpolation/InterpolationTargetDetail.cpp

@@ -5,6 +5,7 @@

 #include "DataStructures/LinkedMessageId.hpp"
 #include "Time/TimeStepId.hpp"
+#include "Utilities/ErrorHandling/Assert.hpp"


Yup still there

It did not include all the maps it needed to. This is a bug fix that should have been done when distorted frame was added to UniformTranslation.

Do this for many classes.

markscheel · 2023-02-22T21:44:13Z

Removed include, rebased and fixed conflict.

wthrowe · 2023-02-22T22:09:46Z

Er, try again? It's still there.

knelli2 · 2023-02-23T01:07:57Z

src/ParallelAlgorithms/Interpolation/InterpolationTargetDetail.cpp

@@ -5,6 +5,7 @@

 #include "DataStructures/LinkedMessageId.hpp"
 #include "Time/TimeStepId.hpp"
+#include "Utilities/ErrorHandling/Assert.hpp"


Yup still there

This enables interpolation in the distorted frame.

Instead, use get_temporal_id_value.

markscheel · 2023-02-23T18:16:07Z

Hopefully the include is gone now... sorry..

knelli2

Yup! Gone now!

markscheel marked this pull request as draft February 10, 2023 15:41

markscheel force-pushed the AhFinderTest branch 2 times, most recently from 67d684f to be39103 Compare February 10, 2023 17:34

markscheel changed the title ~~AhFinder test: enable distorted frame but do not use it.~~ Find apparent horizon in distorted frame Feb 10, 2023

markscheel marked this pull request as ready for review February 10, 2023 17:38

knelli2 self-requested a review February 11, 2023 01:13

markscheel force-pushed the AhFinderTest branch from be39103 to 1acdb28 Compare February 11, 2023 01:19

knelli2 requested changes Feb 11, 2023

View reviewed changes

nilsvu requested changes Feb 11, 2023

View reviewed changes

nilsvu previously approved these changes Feb 17, 2023

View reviewed changes

markscheel dismissed nilsvu’s stale review via f0bf890 February 17, 2023 16:44

markscheel force-pushed the AhFinderTest branch from 8dbddde to f0bf890 Compare February 17, 2023 16:44

knelli2 previously approved these changes Feb 17, 2023

View reviewed changes

nilsvu requested a review from wthrowe February 18, 2023 19:04

nilsvu previously approved these changes Feb 18, 2023

View reviewed changes

wthrowe requested changes Feb 21, 2023

View reviewed changes

knelli2 added this to the BBH inspiral reaches common apparent horizon milestone Feb 22, 2023

markscheel added 2 commits February 22, 2023 13:34

UniformTranslation: fix maps_list.

93d0606

It did not include all the maps it needed to. This is a bug fix that should have been done when distorted frame was added to UniformTranslation.

Add Distorted-frame instantiations.

a8516dc

Do this for many classes.

markscheel dismissed stale reviews from nilsvu and knelli2 via a1a300f February 22, 2023 21:41

markscheel force-pushed the AhFinderTest branch from f0bf890 to a1a300f Compare February 22, 2023 21:41

knelli2 requested changes Feb 23, 2023

View reviewed changes

markscheel added 2 commits February 23, 2023 10:14

compute_dest_vars_from_src_vars: distorted frame.

f023cc7

This enables interpolation in the distorted frame.

Test_AhFinder: test in distorted frame.

f1d9de4

markscheel force-pushed the AhFinderTest branch from a1a300f to 99f42ec Compare February 23, 2023 18:14

Remove evaluate_temporal_id_for_expiration.

53e5363

Instead, use get_temporal_id_value.

markscheel force-pushed the AhFinderTest branch from 99f42ec to 53e5363 Compare February 23, 2023 18:15

knelli2 approved these changes Feb 23, 2023

View reviewed changes

wthrowe approved these changes Feb 23, 2023

View reviewed changes

nilsvu approved these changes Feb 23, 2023

View reviewed changes

nilsvu enabled auto-merge February 23, 2023 20:52

nilsdeppe disabled auto-merge February 24, 2023 07:12

nilsdeppe merged commit 37d7710 into sxs-collaboration:develop Feb 24, 2023

markscheel deleted the AhFinderTest branch February 27, 2023 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Find apparent horizon in distorted frame #4720

Find apparent horizon in distorted frame #4720

markscheel commented Feb 10, 2023 •

edited

knelli2 left a comment

knelli2 Feb 11, 2023

markscheel Feb 13, 2023

wthrowe Feb 13, 2023

markscheel Feb 13, 2023

wthrowe Feb 14, 2023

knelli2 Feb 14, 2023 •

edited

wthrowe Feb 14, 2023

markscheel Feb 15, 2023

knelli2 Feb 15, 2023

wthrowe Feb 15, 2023

markscheel commented Feb 17, 2023

nilsdeppe commented Feb 21, 2023

wthrowe Feb 21, 2023

knelli2 Feb 23, 2023

markscheel commented Feb 22, 2023

wthrowe commented Feb 22, 2023

knelli2 Feb 23, 2023

markscheel commented Feb 23, 2023

knelli2 left a comment

		functions_of_time),
		InterpolationTarget_detail::evaluate_temporal_id_for_expiration(

Find apparent horizon in distorted frame #4720

Find apparent horizon in distorted frame #4720

Conversation

markscheel commented Feb 10, 2023 • edited

Proposed changes

Code review checklist

knelli2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knelli2 Feb 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markscheel commented Feb 17, 2023

nilsdeppe commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markscheel commented Feb 22, 2023

wthrowe commented Feb 22, 2023

Choose a reason for hiding this comment

markscheel commented Feb 23, 2023

knelli2 left a comment

Choose a reason for hiding this comment

markscheel commented Feb 10, 2023 •

edited

knelli2 Feb 14, 2023 •

edited