Alter CCE interface managers for new changes to interpolation #3422

moxcodes · 2021-08-09T22:19:36Z

Proposed changes

This makes a number of changes to the interface for the GH system sending data to CCE based on the newer design considerations from the availability of dense interpolation:

The interface managers no longer use the time derivatives of the GH variables, as they won't be as easily available from dense output
The interface managers are no longer factory-creatable; the GhLockstep interface manager is the only available method during self-start, and the GhLocalTimeStepping interface manager is the only available method during the evolution
Handle self-start separately using an action that manually calls the appropriate interpolation event, and take the TimeStepId as the relevant temporal id for those utilities intended for self start, and double for all other cases.

Upgrade instructions

Any usage of the GhInterfaceManagers will likely need to be reworked to account for the new interfaces.

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.

Dependencies

Specify temporal_id for interpolation in interpolation target tags #3421

moxcodes · 2021-08-13T18:34:49Z

Rebased on develop, no longer dependent.

wthrowe · 2021-08-13T19:11:52Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.cpp

+template <typename VarsToInterpolate>
+auto create_span_for_time_value(
+    double time, size_t interpolator_length,
+    const std::map<double, VarsToInterpolate>& gh_data) noexcept {


How about something like this. (Probably +1 in some places.)

const auto now = gh_data.upper_bound(time); if (std::distance(gh_data.begin(), now) < interpolator_length)) { return gh_data.begin(); } else if (std::distance(now, gh_data.end()) < interpolator_length) { return std::prev(gh_data.end(), 2 * interpolator_length); } else { return std::prev(now, interpolator_length); }

Nice, I like this -- I didn't know std::prev for incrementing/decrementing iterators by arbitrary amounts.

wthrowe · 2021-08-13T19:12:58Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.cpp

+    while (lower_span_size < interpolator_length) {
+      ++upper_iterator;
+      ++lower_span_size;
+    }


If this form of this function survives, this loop should be removed because none of the variables modified here are read afterwards.

wthrowe · 2021-08-13T19:18:44Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.cpp

+      2 * interpolator->required_number_of_points_before_and_after()};
+  DataVector tensor_component_values{
+      2 * interpolator->required_number_of_points_before_and_after()};
+  for (auto [i, map_it] = std::make_tuple(0_st, iterator_start);


(Not a request) Neat! I haven't seen this structured-binding application before.

wthrowe · 2021-08-13T19:30:11Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.cpp

+  for (auto gh_data_it = gh_data_.begin();
+       gh_data_it != gh_data_.end() and
+       gh_data_it->first < requests_.front().substep_time().value();
+       ++gh_data_it, ++number_of_points_before_first_request) {
+  }


Can this be written in terms of upper_bound or something?

wthrowe · 2021-08-13T19:34:13Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.hpp

+ * sufficient to perform interpolation to arbitrary times required by CCE. From
+ * the Generalized Harmonic system, it receives the spacetime metric \f$g_{a
+ * b}\f$ and Generalized Harmonic \f$\Phi_{i a b}\f$ and \f$\Pi_{ab}\f$ and the
+ * current `TimeStepId` via `GhLocalTimeStepping::insert_gh_data()`. The CCE


Not TimeStepId anymore.

wthrowe · 2021-08-13T19:42:09Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.hpp

-  /// system should wait for additional data from the GH system.
-  void insert_gh_data(TimeStepId time_id,
+  /// \brief Store the provided data set to prepare for interpolation.
+  void insert_gh_data(double time,


I don't remember how the code driving this class works, but is there something sequencing the calls to insert_gh_data and retrieve_and_remove_first_ready_gh_data so that you get consistent results when running in parallel?

The way I have it working is that there is an action that inserts requests for the next CCE time, and an action that inserts new data from GH interpolation. Each time there is either a new request or new gh data it checks the retrieve_and_remove_first_ready_gh_data, and sends it to the CCE evolution component if its ready, so I don't think there's any trouble with sequencing between the components.
The GH data is stored in a map, so I think there shouldn't be any trouble with misordered arrival of the GH data.
The CCE requests are stored in a std::deque, so there is an assumption here that the CCE requests are ordered, but I think that is guaranteed by the communication pattern in CCE -- it always consumes a time before requesting the next. For flexibility, though, I'll to change it to a std::set, then there aren't any ordering constraints for these structures.

I think that answers my question, but I'm going to ask another clarifying one to be sure.

The thing I'm worried about is that the create_span_for_time_value function makes decisions based on what GH data is available. Is the set of available data deterministic when that function is called?

Ah, right, that's a good point. That does make ordering assumptions -- In particular, one could imagine that if the data was inserted such that most of the data in a part of the evolution arrives in sequence, but one intermediate value arrives far later, then this interpolation would activate using more distant points, and presumably have degraded accuracy because of the missing point.

I think I can solve this if I can send along with each set of data from a dense trigger, the previous time the dense trigger activated -- then the interpolation step in this class could check that the set of data it is using to perform the interpolation is contiguous.
Is there an easy way to get access to the previous trigger time so that I could include that information?

You can change DenseTrigger::is_triggered to non-const and use a member variable. I was planning to do that anyway because the control system trigger has some similar needs.

wthrowe · 2021-08-13T19:48:52Z

tests/Unit/Evolution/Systems/Cce/Test_OptionTags.cpp

@@ -28,7 +28,9 @@
 #include "Framework/TestCreation.hpp"
 #include "Helpers/DataStructures/DataBox/TestHelpers.hpp"
 #include "Helpers/Evolution/Systems/Cce/BoundaryTestHelpers.hpp"
+#include "NumericalAlgorithms/Interpolation/BarycentricRationalSpanInterpolator.hpp"


Why are these needed?

I'll drop these from this commit directly -- looks like they're removed again in a later commit anyway.

moxcodes · 2021-08-14T20:37:04Z

fixup posted.
Cheers!

wthrowe · 2021-08-16T18:41:56Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.hpp

@@ -110,7 +110,7 @@ class GhLocalTimeStepping : public GhInterfaceManager {
  void clean_up_gh_data() noexcept;

  std::map<double, gh_variables> gh_data_;
-  std::deque<TimeStepId> requests_;
+  std::set<TimeStepId> requests_;


I think std::priority_queue is that data structure you're looking for, unless you need to ignore duplicates for some reason.

Ah, the std::priority_queue turns out to be troublesome for serialization, because as far as I can tell, it's a non-iterable container. The only way I can see to serialize it is to either make a full copy and serialize as elements are popped off the top, or to pop elements, save them off to a different container, then push them back on again (so, again, requires something like a copy).

If it's acceptable for the time being, I think I'd prefer to leave it as a set to make the serialization more graceful.

Ah, yeah, that's a good reason. I expect this container should always be small anyway.

wthrowe · 2021-08-16T18:43:48Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLockstep.hpp

@@ -85,7 +85,7 @@ class GhLockstep : public GhInterfaceManager {

 private:
  std::map<TimeStepId, gh_variables> provided_data_;
-  std::deque<TimeStepId> requests_;
+  std::set<TimeStepId> requests_;


priority_queue again.

wthrowe · 2021-08-16T18:54:19Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.hpp

-  /// system should wait for additional data from the GH system.
-  void insert_gh_data(TimeStepId time_id,
+  /// \brief Store the provided data set to prepare for interpolation.
+  void insert_gh_data(double time,


I think that answers my question, but I'm going to ask another clarifying one to be sure.

The thing I'm worried about is that the create_span_for_time_value function makes decisions based on what GH data is available. Is the set of available data deterministic when that function is called?

wthrowe · 2021-08-18T18:25:02Z

You can squash and rebase.

moxcodes · 2021-08-19T04:53:28Z

I've squashed the previous fixup and rebased. I've added two new commits and a fixup that make the remaining changes to the interface managers necessary to guarantee strict ordering using the previous times from the triggers.
The new commits are:
Add pup function to circumvent charm bug for map with comparator and Add TimeAndPrevious for use as an interpolator temporal id.
I need to send the previous time packaged in the temporal id, because currently interpolators do not support including with the interpolated data with anything that isn't interpolated on the mesh -- perhaps that strategy isn't the most elegant, but it was the best way I could think of to minimize the scope of additional code changes.
Cheers!

wthrowe · 2021-08-20T01:23:36Z

tests/Unit/Parallel/Test_PupStlCpp11.cpp

+    std::map<std::pair<int, double>, double, PairComparator> map_to_serialize;
+    map_to_serialize.insert({std::make_pair(1, 2.0), 3.0});
+    map_to_serialize.insert({std::make_pair(3, 1.0), 1.5});
+    map_to_serialize.insert({std::make_pair(2, 6.0), 10.2});


I'm not sure what this commit is about (maybe give a brief description somewhere?), but this test is definitely not doing what you intend. The statement on this line doesn't do anything because it is a "duplicate" of the first key that was inserted.

Oh, yeah, I see the mistake; I guess I shouldn't have gotten cute with the comparison operator -- I'll fix the comparator.
The commit is just adding a pup function that a) doesn't interfere with existing pup functions for STL containers -- because we'll want to use the charm built-in one once it works, and b) doesn't cause a compiler error for a std::map with a custom comparator. There is currently a charm bug (actually just fixed, but not yet in a version) that causes reserve to be called when the map has a custom comparator, and therefore doesn't compile, I'll add that information to the commit message as well

edit: ~~if you think it would be better to just make this the actual pup function for std::map with a comparator and then remove it later when the charm fix is available, I'm fine with doing that too~~ nevermind, that's redefinition problem, needs to be left as a separate function

OK, I see. I think you could add an overload of PUP::reserve_if_applicable, but I like the separate function better.

(It was a valid comparator, just probably not one you'd want for a general map. I can even assign it almost-reasonable meaning as the timelike (i.e., non-null) comparator for events in null coordinates in SR with one axis reversed. :))

wthrowe · 2021-08-20T01:27:55Z

src/Evolution/Systems/Cce/InterfaceManagers/GhLocalTimeStepping.hpp

 #include "Time/TimeStepId.hpp"
 #include "Time/TimeSteppers/AdamsBashforthN.hpp"
+#include "Utilities/MakeString.hpp"


wthrowe

Squash.

wthrowe · 2021-08-20T14:01:52Z

tests/Unit/Parallel/Test_PupStlCpp11.cpp

+    std::map<std::pair<int, double>, double, PairComparator> map_to_serialize;
+    map_to_serialize.insert({std::make_pair(1, 2.0), 3.0});
+    map_to_serialize.insert({std::make_pair(3, 1.0), 1.5});
+    map_to_serialize.insert({std::make_pair(2, 6.0), 10.2});


OK, I see. I think you could add an overload of PUP::reserve_if_applicable, but I like the separate function better.

(It was a valid comparator, just probably not one you'd want for a general map. I can even assign it almost-reasonable meaning as the timelike (i.e., non-null) comparator for events in null coordinates in SR with one axis reversed. :))

moxcodes · 2021-08-20T21:25:08Z

fixups squashed in.
Cheers!

moxcodes · 2021-08-21T01:20:14Z

Needed to squash again to remove a std::move clang-tidy had complained about.
Cheers!

wthrowe · 2021-08-27T15:56:59Z

(Not a request.) We've encountered a similar requirement for keeping track of message order in the control systems, which I'll probably try to deal with something like how it was done here. If I end up with a sufficiently general solution it might be possible to share some classes between the two, but I don't know if that will happen, yet. Not something that should hold this PR up.

They will not be needed anymore, as CCE will not support any method of data transfer apart from dense interpolation.

Adds a function `pup_override` to use for `std::map`s with custom comparators, because currently a charm bug causes an invalid `reserve` call for such containers. The function is intentionally chosen to not directly interfere with the charm pup override so that the charm version of the function can be easily used or tested once the bug is fixed.

- dense interpolation sends `double`s for times rather than TimeStepIds - dense interpolation will not send time derivatives of the evolved quantities, so we must perform the CCE interpolation without them.

Both versions of interface manager will still be needed, and need to maintain a common interface, but they should not be selectable at runtime -- one will be used only during self-start, and one only during the main evolution.

Interpolation for self start cannot follow the standard events and dense triggers control flow, so needs a specialized action to provide CCE with boundary data during self start.

This is needed to provide the interpolator source tags for CCE

Self start with GH-CCE requires the communication via the interpolator, which requires registration. It should be fine to perform registration before self-start, as no current registration process assumes anything about the state of the `TimeStepId`.

This allows evaluating dense triggers at a step value shortly after self-start.

kidder · 2021-09-29T00:33:02Z

@wthrowe I rebased this

wthrowe · 2021-09-29T01:33:12Z

I need to think for a bit about whether there is any interaction with #3510.

wthrowe · 2021-09-30T19:22:37Z

I think "Use evolution_less_equal for receive_local_time_stepping" is redundant with #3510 and can be dropped. Unfortunately, I can't remember exactly what problem that commit was supposed to fix. (I was consulted on the matter, but I don't remember what the problem was anymore.)

moxcodes · 2021-10-16T19:39:37Z

@wthrowe Yes, I think you're correct that the fix in #3510 corrects the same problem as "Use evolution_less_equal for receive_local_time_stepping". My suspicion is that commit can be safely dropped, though we'll have to test the CCE communication use-case to be sure.

kidder · 2022-05-16T19:28:09Z

I took over this PR in #3963
The commit Use evolution_less_equal for receive_local_time_stepping was not used

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

PR sxs-collaboration#3422

26b717a

moxcodes mentioned this pull request Aug 10, 2021

CCE-GH executable #2323

Closed

16 tasks

moxcodes force-pushed the dense_trigger_interface_manager branch from 8c79776 to 105b9b8 Compare August 10, 2021 01:24

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

PR sxs-collaboration#3422

bf9f660

moxcodes force-pushed the dense_trigger_interface_manager branch from 105b9b8 to dfaae3d Compare August 13, 2021 18:34

wthrowe requested changes Aug 13, 2021

View reviewed changes

moxcodes force-pushed the dense_trigger_interface_manager branch from dfaae3d to 8d144c2 Compare August 14, 2021 20:36

wthrowe requested changes Aug 16, 2021

View reviewed changes

moxcodes mentioned this pull request Aug 17, 2021

Add previous trigger time to dense triggers #3445

Merged

3 tasks

moxcodes force-pushed the dense_trigger_interface_manager branch from 8d144c2 to 0616dd7 Compare August 19, 2021 04:49

wthrowe requested changes Aug 20, 2021

View reviewed changes

moxcodes force-pushed the dense_trigger_interface_manager branch from 0616dd7 to 87e0264 Compare August 20, 2021 06:31

wthrowe reviewed Aug 20, 2021

View reviewed changes

moxcodes force-pushed the dense_trigger_interface_manager branch from 87e0264 to c86cb80 Compare August 20, 2021 21:24

wthrowe previously approved these changes Aug 20, 2021

View reviewed changes

moxcodes dismissed wthrowe’s stale review via b7b4a65 August 21, 2021 01:19

moxcodes force-pushed the dense_trigger_interface_manager branch from c86cb80 to b7b4a65 Compare August 21, 2021 01:19

wthrowe previously approved these changes Aug 23, 2021

View reviewed changes

wthrowe mentioned this pull request Aug 31, 2021

Add LinkedMessageQueue #3479

Merged

3 tasks

moxcodes added 4 commits September 28, 2021 14:56

Remove InterpolationStrategies

3f23a8a

They will not be needed anymore, as CCE will not support any method of data transfer apart from dense interpolation.

Add TimeAndPrevious for use as an interpolator temporal id

cc66e46

Alter GhInterfaceManagers to fit dense interpolation

b1d74df

- dense interpolation sends `double`s for times rather than TimeStepIds - dense interpolation will not send time derivatives of the evolved quantities, so we must perform the CCE interpolation without them.

Remove factory-creation from GhInterfaceManagers

4728e1c

Both versions of interface manager will still be needed, and need to maintain a common interface, but they should not be selectable at runtime -- one will be used only during self-start, and one only during the main evolution.

kidder dismissed wthrowe’s stale review via 7205109 September 28, 2021 19:38

kidder force-pushed the dense_trigger_interface_manager branch from b7b4a65 to 7205109 Compare September 28, 2021 19:38

moxcodes added 6 commits September 28, 2021 16:05

Add InterpolateDuringSelfStart specialized action for CCE-GH

a755f13

Interpolation for self start cannot follow the standard events and dense triggers control flow, so needs a specialized action to provide CCE with boundary data during self start.

Relax constraints on source tags for Horizon

dcd32c5

This is needed to provide the interpolator source tags for CCE

Use self-start specific interface manager during self-start

97b7957

Use evolution_less_equal for receive_local_time_stepping

d9eb48d

This allows evaluating dense triggers at a step value shortly after self-start.

Don't send time derivatives for CCE interpolation

894fe69

kidder force-pushed the dense_trigger_interface_manager branch from 7205109 to 894fe69 Compare September 28, 2021 20:05

kidder approved these changes Sep 29, 2021

View reviewed changes

wthrowe mentioned this pull request Oct 20, 2021

Remove uses of TimeStepId in interpolation #3592

Closed

3 tasks

kidder mentioned this pull request May 7, 2022

Alter CCE interface managers for new changes to interpolation #3963

Merged

3 tasks

kidder closed this May 16, 2022

kidder mentioned this pull request Jun 2, 2023

Somewhat fix ordering issues in interpolator #5070

Merged

3 tasks

Alter CCE interface managers for new changes to interpolation #3422

Alter CCE interface managers for new changes to interpolation #3422

Conversation

moxcodes commented Aug 9, 2021 • edited

Proposed changes

Upgrade instructions

Code review checklist

Dependencies

moxcodes commented Aug 13, 2021

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

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 14, 2021

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

wthrowe commented Aug 18, 2021

moxcodes commented Aug 19, 2021

Choose a reason for hiding this comment

moxcodes Aug 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moxcodes commented Aug 20, 2021

moxcodes commented Aug 21, 2021

wthrowe commented Aug 27, 2021

kidder commented Sep 29, 2021

wthrowe commented Sep 29, 2021

wthrowe commented Sep 30, 2021

moxcodes commented Oct 16, 2021

kidder commented May 16, 2022

moxcodes commented Aug 9, 2021 •

edited

moxcodes Aug 20, 2021 •

edited