Specify `temporal_id` for interpolation in interpolation target tags #3421

moxcodes · 2021-08-09T21:38:24Z

Proposed changes

Apologies to @wthrowe , whose commit I found I needed to revert to make this work.

To construct the coupled CCE-GH system without fairly radical rethinking of the communication pattern, we need to perform interpolation during self-start, and that can only be accomplished unambigously if we use a key for the interpolator maps that distinguishes between self-start stage -- so, a double that represents the current time won't work.

The control-flow of interpolation during self start will not use dense triggers.

To support the events and dense triggers needed for most use-cases, I've changed the interpolation framework to use a tag specified by the temporal_id type alias that must now be specified in the interpolation target tags. For almost all cases, that should be ::Tags::Time, but for interpolation during self start can be ::Tags::TimeStepId.

Upgrade instructions

Interpolation target tags must now specify a temporal_id. In almost all cases it should be using temporal_id = ::Tags::Time;.

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.

This reverts commit 439a996.

wthrowe · 2021-08-10T16:54:13Z

src/NumericalAlgorithms/Interpolation/InterpolationTargetDetail.cpp

+double get_temporal_id_value(const TimeStepId time_id) noexcept {
+  return time_id.substep_time().value();
+}
+double evaluate_temporal_id_for_expiration(double time) noexcept {
+  return time;
+}
+double evaluate_temporal_id_for_expiration(TimeStepId time_id) noexcept {
+  return time_id.step_time().value();
+}


const&
const
const&

wthrowe · 2021-08-10T16:55:32Z

src/NumericalAlgorithms/Interpolation/InterpolationTargetDetail.hpp

+double get_temporal_id_value(const double time) noexcept;
+double get_temporal_id_value(const TimeStepId time_id) noexcept;
+double evaluate_temporal_id_for_expiration(const double time) noexcept;
+double evaluate_temporal_id_for_expiration(const TimeStepId time_id) noexcept;


No const on the double versions. (The others should change with my previous comment.)

wthrowe · 2021-08-10T17:04:44Z

tests/Unit/ApparentHorizons/Test_ApparentHorizonFinder.cpp

@@ -227,6 +227,7 @@ template <typename PostHorizonFindCallback, typename IsTimeDependent,
 struct MockMetavariables {
  static constexpr bool use_time_dependent_maps = IsTimeDependent::value;
  struct AhA {
+    using temporal_id = ::Tags::Time;


Drop the alias below in MockMetavariables.

wthrowe · 2021-08-10T17:04:57Z

tests/Unit/ApparentHorizons/Test_ApparentHorizonFinder.cpp

@@ -350,9 +351,7 @@ void test_apparent_horizon(const gsl::not_null<size_t*> test_horizon_called,
  // zero iterations.
  // Having two temporal_ids tests some logic in the interpolator.
  Slab slab(0.0, 1.0);


wthrowe · 2021-08-10T17:06:24Z

tests/Unit/ApparentHorizons/Test_ApparentHorizonFinder.cpp

@@ -124,7 +124,7 @@ struct TestSchwarzschildHorizon {
  template <typename DbTags, typename Metavariables>


I don't think anything except Time/Tags.hpp should be included from Time.

wthrowe · 2021-08-10T17:09:17Z

tests/Unit/Evolution/Systems/Cce/Actions/Test_SendNextTimeToCce.cpp

@@ -88,6 +88,7 @@ struct mock_gh_worldtube_boundary {
 struct test_metavariables {


This file is missing several Time includes. I think Slab, Tags, Time, TimeStepId.

wthrowe · 2021-08-10T17:10:30Z

tests/Unit/Helpers/NumericalAlgorithms/Interpolation/InterpolateOnElementTestHelpers.hpp

+  } else {
+    (void)temporal_id;
+    ERROR("Unsupported temporal id type: " << pretty_type::short_name<
+              typename metavars::InterpolationTargetA::temporal_id::type>());


static_assert?

wthrowe · 2021-08-10T17:13:22Z

tests/Unit/Helpers/NumericalAlgorithms/Interpolation/InterpolationTargetTestHelpers.hpp

+  } else {
+    (void)temporal_id;
+    ERROR("Unsupported temporal id type: "
+          << pretty_type::short_name<temporal_id_type>());


static_assert?

wthrowe · 2021-08-10T17:20:33Z

tests/Unit/NumericalAlgorithms/Interpolation/Test_InterpolationTargetReceiveVars.cpp

-    CHECK(temporal_id == TimeStepId(true, 0, Time(slab, Rational(14, 15))));
+    CHECK(temporal_id == TimeStepId(true, 0, Time(slab, Rational(14, 15)))
+                             .substep_time()
+                             .value());


14.0 / 15.0

Same elsewhere, drop any unnecessary includes. If you're concerned about roundoff you can switch to integral values or binary fractions.

moxcodes · 2021-08-10T18:29:14Z

fixup posted.
Cheers!

wthrowe · 2021-08-10T21:36:42Z

Looks good. Squash.

moxcodes · 2021-08-10T22:28:06Z

Squashed.
Cheers!

kidder

only half a review so far; will review changes to tests tomorrow

src/Evolution/Executables/GeneralizedHarmonic/EvolveGeneralizedHarmonicWithHorizon.hpp

src/Evolution/Executables/GeneralizedHarmonic/GeneralizedHarmonicBase.hpp

src/Evolution/Executables/GrMhd/GhValenciaDivClean/EvolveGhValenciaDivCleanWithHorizon.hpp

src/NumericalAlgorithms/Interpolation/InitializeInterpolator.hpp

src/NumericalAlgorithms/Interpolation/Interpolate.hpp

src/NumericalAlgorithms/Interpolation/InterpolationTargetDetail.cpp

src/NumericalAlgorithms/Interpolation/Tags.hpp

moxcodes · 2021-08-11T06:05:29Z

@kidder I think several of the comments are regarding the Revert commit -- I'm happy to make the suggested tweaks in a separate commit, but I think we should preserve the git-generated revert as a more honest representation of the commit history (reading a log, I'd expect the Revert to precisely reverse the changes for the specified commit).

kidder · 2021-08-11T06:34:56Z

sure, that is fine

kidder

I've gone through the tests and have no more requested changes

moxcodes · 2021-08-11T22:00:42Z

posted a fixup commit and the new commit Clean up following reversion to temporal_id for interpolation for the suggested changes to the Revert commit.
Cheers!

kidder

you can squash

moxcodes · 2021-08-12T05:19:38Z

fixup squashed in.
Cheers!

kidder · 2021-08-13T17:25:25Z

@wthrowe you can merge this if you are happy with it

Revert "Convert interpolation indexing to doubles"

652426b

This reverts commit 439a996.

moxcodes added a commit to moxcodes/spectre that referenced this pull request Aug 9, 2021

PR sxs-collaboration#3421

df41f44

moxcodes requested review from wthrowe and markscheel August 9, 2021 22:09

moxcodes added a commit to moxcodes/spectre that referenced this pull request Aug 9, 2021

PR sxs-collaboration#3421

8b66fa3

This was referenced Aug 9, 2021

Alter CCE interface managers for new changes to interpolation #3422

Closed

CCE-GH executable #2323

Closed

wthrowe requested changes Aug 10, 2021

View reviewed changes

moxcodes force-pushed the temporal_ids_from_interpolation_targets branch from 62eea1d to ae13208 Compare August 10, 2021 22:27

kidder requested changes Aug 11, 2021

View reviewed changes

kidder reviewed Aug 11, 2021

View reviewed changes

kidder reviewed Aug 12, 2021

View reviewed changes

moxcodes added 2 commits August 11, 2021 22:12

Use temporal_id from InterpolationTargetTags

f29ecf6

Clean up following reversion to temporal_id for interpolation

cb92120

moxcodes force-pushed the temporal_ids_from_interpolation_targets branch from c69dda6 to cb92120 Compare August 12, 2021 05:12

kidder approved these changes Aug 12, 2021

View reviewed changes

wthrowe approved these changes Aug 13, 2021

View reviewed changes

wthrowe merged commit d97dc8a into sxs-collaboration:develop Aug 13, 2021

nilsvu changed the title ~~specify temporal_id for interpolation in interpolation target tags~~ Specify temporal_id for interpolation in interpolation target tags Sep 5, 2021

markscheel mentioned this pull request Oct 20, 2021

Remove uses of TimeStepId in interpolation #3592

Closed

3 tasks

knelli2 mentioned this pull request Feb 14, 2023

Find apparent horizon in distorted frame #4720

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify `temporal_id` for interpolation in interpolation target tags #3421

Specify `temporal_id` for interpolation in interpolation target tags #3421

moxcodes commented Aug 9, 2021

wthrowe Aug 10, 2021

wthrowe Aug 10, 2021

wthrowe Aug 10, 2021

wthrowe Aug 10, 2021

wthrowe Aug 10, 2021

wthrowe Aug 10, 2021

wthrowe Aug 10, 2021

wthrowe Aug 10, 2021

wthrowe Aug 10, 2021

moxcodes commented Aug 10, 2021

wthrowe commented Aug 10, 2021

moxcodes commented Aug 10, 2021

kidder left a comment

moxcodes commented Aug 11, 2021

kidder commented Aug 11, 2021

kidder left a comment

moxcodes commented Aug 11, 2021

kidder left a comment

moxcodes commented Aug 12, 2021

kidder commented Aug 13, 2021

		@@ -124,7 +124,7 @@ struct TestSchwarzschildHorizon {
		template <typename DbTags, typename Metavariables>

		@@ -88,6 +88,7 @@ struct mock_gh_worldtube_boundary {
		struct test_metavariables {

Specify temporal_id for interpolation in interpolation target tags #3421

Specify temporal_id for interpolation in interpolation target tags #3421

Conversation

moxcodes commented Aug 9, 2021

Proposed changes

Upgrade instructions

Code review checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moxcodes commented Aug 10, 2021

wthrowe commented Aug 10, 2021

moxcodes commented Aug 10, 2021

kidder left a comment

Choose a reason for hiding this comment

moxcodes commented Aug 11, 2021

kidder commented Aug 11, 2021

kidder left a comment

Choose a reason for hiding this comment

moxcodes commented Aug 11, 2021

kidder left a comment

Choose a reason for hiding this comment

moxcodes commented Aug 12, 2021

kidder commented Aug 13, 2021

Specify `temporal_id` for interpolation in interpolation target tags #3421

Specify `temporal_id` for interpolation in interpolation target tags #3421