Add FoTAreReady function for simple actions #5513

knelli2 · 2023-09-28T21:12:09Z

Proposed changes

The previous version of functions_of_time_are_ready() only used a PerformAlgorithmCallback. But sometimes we want to use a SimpleActionCallback. This generalizes functions_of_time_are_ready() to allow both.

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

You also have some clang-tidy warnings. The gcc9 failures appear to be unrelated and will hopefully work after a new push.

wthrowe · 2023-09-28T22:06:30Z

src/Domain/FunctionsOfTime/FunctionsOfTimeAreReady.hpp

+/// are moved into the callback.
+template <typename CacheTag, typename SimpleAction, typename Metavariables,
+          typename ArrayIndex, typename Component, typename... Args>
+bool functions_of_time_are_ready(


Not sure I'm a fan of distinguishing the two overloads only by the existence of a template parameter.

Any suggestions? I did it like this because the different callbacks have different template parameters. Didn't make a lot of sense to have to specify an extra template for the perform algorithm callback that would never be used.

Give them different names, maybe.

wthrowe · 2023-09-28T22:07:29Z

src/Domain/FunctionsOfTime/FunctionsOfTimeAreReady.hpp

+    Parallel::GlobalCache<Metavariables>& cache, const ArrayIndex& array_index,
+    const Component* component_p, const double time,
+    const std::optional<std::unordered_set<std::string>>& functions_to_check,
+    Args... args) {


Args&& and forward below.

I tried this first, but ran into build errors in SimpleActionCallback due to the const &ness of some of the args that were passed in. I wasn't sure how to get around that because SimpleActionCallback needs to actually store the data locally (not references to it which may go out of scope). That's why I chose a copy initially and then std::move from then on.

That's a bug in SimpleActionCallback. It shouldn't be taking it's arguments by reference, but by value and then moving.

Ok added a new commit that fixes SimpleActionCallback

wthrowe · 2023-09-28T22:08:47Z

src/Domain/FunctionsOfTime/FunctionsOfTimeAreReady.hpp

+    Parallel::GlobalCache<Metavariables>& cache, const ArrayIndex& array_index,
+    const Component* /*meta*/, const double time,
+    const std::optional<std::unordered_set<std::string>>& functions_to_check,
+    Args... args) {


Args&& and forward below.

wthrowe · 2023-09-29T17:26:36Z

src/Parallel/Callback.hpp

@@ -37,9 +37,10 @@ class SimpleActionCallback : public Callback {
 public:
  WRAPPED_PUPable_decl_template(SimpleActionCallback);  // NOLINT
  SimpleActionCallback() = default;
-  SimpleActionCallback(Proxy proxy, Args&&... args)
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  SimpleActionCallback(Proxy proxy, std::decay_t<Args>... args)


Why do you need decay_t all over the place in this class?

Well Args... keeps all the const & so I had to remove those for the declaration of the member tuple so it wasn't a tuple of references, but I left everything else as Args.... However, my test failed with that because a DataVector I was using to test actually got moved into this callback instead of copied (not std::move from the calling code, but from the std::move one line below) and this caused subsequent tests to fail. When I did std::decay_t<Args>... in the constructor, everything worked fine.

I see. I think you're right and this is the correct way to do it here.

Optionally, you might want to add decay_t to the calling code as well, just to keep the number of equivalent types the compiler has to deal with down. Probably not many instantiations of this class though, so not very important.

Other than the test, the calling code (in the FoT are ready functions) is usually std::forward<Args>(args).... So I'll leave that as is.

wthrowe · 2023-09-29T17:27:03Z

src/Parallel/Callback.hpp

      : proxy_(proxy),
-        args_(std::make_tuple<Args...>(std::forward<Args>(args)...)) {}
+        args_(std::make_tuple<std::decay_t<Args>...>(std::move(args)...)) {}


[optional] make_tuple doesn't do anything here. It can be removed.

wthrowe

That leaves only optional things. Let me know if you want to do any or if this is ready.

wthrowe · 2023-09-29T18:12:12Z

src/Parallel/Callback.hpp

@@ -37,9 +37,10 @@ class SimpleActionCallback : public Callback {
 public:
  WRAPPED_PUPable_decl_template(SimpleActionCallback);  // NOLINT
  SimpleActionCallback() = default;
-  SimpleActionCallback(Proxy proxy, Args&&... args)
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  SimpleActionCallback(Proxy proxy, std::decay_t<Args>... args)


I see. I think you're right and this is the correct way to do it here.

Optionally, you might want to add decay_t to the calling code as well, just to keep the number of equivalent types the compiler has to deal with down. Probably not many instantiations of this class though, so not very important.

knelli2 · 2023-09-29T18:31:41Z

Squashed in the optional remove of std::make_tuple

knelli2 requested a review from wthrowe September 28, 2023 21:12

knelli2 force-pushed the simple_action_fot_ready branch 2 times, most recently from a763611 to 1f3c8da Compare September 28, 2023 21:38

wthrowe requested changes Sep 28, 2023

View reviewed changes

knelli2 force-pushed the simple_action_fot_ready branch 2 times, most recently from 41cb954 to e5c4c68 Compare September 29, 2023 08:05

wthrowe reviewed Sep 29, 2023

View reviewed changes

knelli2 added 2 commits September 29, 2023 11:24

Move args in SimpleActionCallback

fe25ec8

Add FoTAreReady function for simple actions

d8969fb

knelli2 force-pushed the simple_action_fot_ready branch from e5c4c68 to d8969fb Compare September 29, 2023 18:30

wthrowe approved these changes Sep 29, 2023

View reviewed changes

wthrowe merged commit 626e0da into sxs-collaboration:develop Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FoTAreReady function for simple actions #5513

Add FoTAreReady function for simple actions #5513

knelli2 commented Sep 28, 2023

wthrowe left a comment

wthrowe Sep 28, 2023

knelli2 Sep 28, 2023

wthrowe Sep 29, 2023

wthrowe Sep 28, 2023

knelli2 Sep 28, 2023

wthrowe Sep 29, 2023

knelli2 Sep 29, 2023

wthrowe Sep 28, 2023

wthrowe Sep 29, 2023

knelli2 Sep 29, 2023

wthrowe Sep 29, 2023

knelli2 Sep 29, 2023

wthrowe Sep 29, 2023

wthrowe left a comment

wthrowe Sep 29, 2023

knelli2 commented Sep 29, 2023

Add FoTAreReady function for simple actions #5513

Add FoTAreReady function for simple actions #5513

Conversation

knelli2 commented Sep 28, 2023

Proposed changes

Upgrade instructions

Code review checklist

Further comments

wthrowe 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

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

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knelli2 commented Sep 29, 2023