Remove uses of TimeStepId in interpolation #3592

wthrowe · 2021-10-11T20:37:01Z

TimeStepId is not a global identifier, so it cannot be used as an ID
for the interpolator component.

Proposed changes

Upgrade instructions

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.

Further comments

wthrowe · 2021-10-11T20:37:53Z

src/ParallelAlgorithms/Interpolation/InterpolationTargetDetail.hpp

@@ -63,9 +63,7 @@ namespace intrp {

 namespace InterpolationTarget_detail {
 double get_temporal_id_value(double time);
-double get_temporal_id_value(const TimeStepId& time_id);
 double evaluate_temporal_id_for_expiration(double time);


@markscheel What are these two functions? Do we need both of them?

These two functions were added by @moxcodes in #3421 so that certain code would work both for double and for TimeStepId, without writing two different versions of that code.

Jordan needed temporal_id to be something different than a double so that self-start would work with #2323 .

If the temporal_id is now always a double, then these two functions should be removed.

Maybe we should hold off until #3422 is merged so we can see if there's any interaction?

wthrowe · 2021-10-11T20:46:08Z

There are some more of these that I missed.

wthrowe · 2021-10-11T21:55:51Z

Got the rest.

nilsvu · 2021-10-13T16:58:44Z

src/ParallelAlgorithms/Interpolation/InterpolationTargetDetail.cpp

@@ -3,15 +3,7 @@

 #include "ParallelAlgorithms/Interpolation/InterpolationTargetDetail.hpp"

-#include "Time/TimeStepId.hpp"


Nice, I believe this removes the dependence of ParallelInterpolation on Time. Could you double-check, and then remove the dependency from the CMakeLists.txt?

Yep, looks like that's the last one for the main library. The test library still uses it, though. Squashed directly.

TimeStepId is not a global identifier, so it cannot be used as an ID for the interpolator component.

wthrowe · 2021-10-19T23:13:19Z

@markscheel ping

markscheel

A minor comment about a variable name.

My biggest concern is that in #3421 @moxcodes reverted a PR similar to this one because he needed interpolation during the self-start of GH+CCE, and he couldn't use double as a time identifier. Has that changed? And how does this PR impact #2323 ?

markscheel · 2021-10-20T21:21:47Z

tests/Unit/ParallelAlgorithms/Interpolation/Test_CleanUpInterpolator.cpp

@@ -37,13 +35,17 @@ namespace intrp {

 namespace {

+struct OtherId : db::SimpleTag {


Why is this called OtherId? Why not TimeId or something that has Time in it?

The one it is "other" to is called "Time", so using "Time" and "TimeId" as the two IDs seemed rather confusing. I could go with "Time2" or something?

Looking at the code again, I believe that the only reason that there are 2 tags in this test is to test that interpolation works with both TimeStepId and double. So if interpolation only works with double, then I think there doesn't need to be a 2nd tag at all.

markscheel · 2021-10-20T21:22:31Z

tests/Unit/ParallelAlgorithms/Interpolation/Test_InitializeInterpolator.cpp

@@ -25,6 +26,10 @@ class DataVector;

 namespace {

+struct OtherId : db::SimpleTag {


TimeId? TimeTag?

wthrowe

Yes, it looks like some of this may still be needed. I really want to get rid of the non-CCE stuff, though, since using it that way is wrong and it keeps getting copy-pasted to more places.

wthrowe · 2021-10-20T21:59:52Z

tests/Unit/ParallelAlgorithms/Interpolation/Test_CleanUpInterpolator.cpp

@@ -37,13 +35,17 @@ namespace intrp {

 namespace {

+struct OtherId : db::SimpleTag {


The one it is "other" to is called "Time", so using "Time" and "TimeId" as the two IDs seemed rather confusing. I could go with "Time2" or something?

markscheel · 2021-10-21T15:37:30Z

Currently, I think the idea is that you are supposed to use doubles instead of TimeStepIds everywhere except in CCE-GH self-start (and interpolation currently works with both doubles and TimeStepIds). But yes it is annoying that people keep using TimeStepIds in most places (probably my fault, because the original version of interpolation used only TimeStepIds and not doubles).

wthrowe · 2021-11-01T18:57:07Z

In addition to needing to figure out CCE, this is on hold until the interactions between the control system and AH finder have been worked out to avoid merge conflicts.

wthrowe · 2022-08-17T20:14:19Z

This would need to be done very differently to work with CCE stuff. I may revisit this at some point, but I think it will be as easy to start from scratch.

wthrowe commented Oct 11, 2021

View reviewed changes

wthrowe added the in progress Don't review, used for sharing code and getting feedback label Oct 11, 2021

wthrowe force-pushed the remove_interpolate_time_step_id branch from b72541f to fb4ecfe Compare October 11, 2021 21:55

wthrowe removed the in progress Don't review, used for sharing code and getting feedback label Oct 11, 2021

wthrowe force-pushed the remove_interpolate_time_step_id branch from fb4ecfe to c15bd06 Compare October 11, 2021 22:51

nilsdeppe requested a review from markscheel October 11, 2021 23:37

nilsvu reviewed Oct 13, 2021

View reviewed changes

Remove uses of TimeStepId in interpolation

9b914e9

TimeStepId is not a global identifier, so it cannot be used as an ID for the interpolator component.

wthrowe force-pushed the remove_interpolate_time_step_id branch from c15bd06 to 9b914e9 Compare October 13, 2021 17:50

nilsvu approved these changes Oct 14, 2021

View reviewed changes

markscheel reviewed Oct 20, 2021

View reviewed changes

wthrowe commented Oct 20, 2021

View reviewed changes

wthrowe added the in progress Don't review, used for sharing code and getting feedback label Nov 1, 2021

wthrowe closed this Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove uses of TimeStepId in interpolation #3592

Remove uses of TimeStepId in interpolation #3592

wthrowe commented Oct 11, 2021

wthrowe Oct 11, 2021

markscheel Oct 20, 2021 •

edited

wthrowe Oct 20, 2021

wthrowe commented Oct 11, 2021

wthrowe commented Oct 11, 2021

nilsvu Oct 13, 2021

wthrowe Oct 13, 2021

wthrowe commented Oct 19, 2021

markscheel left a comment

markscheel Oct 20, 2021

wthrowe Oct 20, 2021

markscheel Oct 21, 2021

markscheel Oct 20, 2021

wthrowe left a comment

wthrowe Oct 20, 2021

markscheel commented Oct 21, 2021

wthrowe commented Nov 1, 2021

wthrowe commented Aug 17, 2022

		@@ -3,15 +3,7 @@

		#include "ParallelAlgorithms/Interpolation/InterpolationTargetDetail.hpp"

		#include "Time/TimeStepId.hpp"

		@@ -37,13 +35,17 @@ namespace intrp {

		namespace {

		struct OtherId : db::SimpleTag {

		@@ -25,6 +26,10 @@ class DataVector;

		namespace {

		struct OtherId : db::SimpleTag {

Remove uses of TimeStepId in interpolation #3592

Remove uses of TimeStepId in interpolation #3592

Conversation

wthrowe commented Oct 11, 2021

Proposed changes

Upgrade instructions

Code review checklist

Further comments

Choose a reason for hiding this comment

markscheel Oct 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wthrowe commented Oct 11, 2021

wthrowe commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wthrowe commented Oct 19, 2021

markscheel 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

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markscheel commented Oct 21, 2021

wthrowe commented Nov 1, 2021

wthrowe commented Aug 17, 2022

markscheel Oct 20, 2021 •

edited