Add IMEX SolveImplicitSector mutator #5653

wthrowe · 2023-12-05T21:12:24Z

This is the actual solver for the IMEX code. The remaining stuff to submit is mostly actions and other similar wrappers.

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

nilsdeppe

This is great! Thanks for all your hard work on the IMEX code! I have a few suggestions/comments/questions. Please feel free to rebase and squash directly since I don't think any of them are major.

nilsdeppe · 2023-12-07T18:12:29Z

src/Evolution/Imex/Tags/Mode.hpp

+
+namespace imex {
+namespace OptionTags {
+struct ImexMode {


Do you want to add Doxygen comments for the tags, mostly for discoverability?

nilsdeppe · 2023-12-07T18:14:26Z

src/Evolution/Imex/Tags/Mode.hpp

+struct ImexMode {
+  static constexpr Options::String help{"IMEX implementation to use"};
+  using type = ::imex::Mode;
+  using group = evolution::OptionTags::Group;


Would it be useful to add an Imex subgroup into Evolution to group the options together? [later in a separate PR is totally fine if you think it's a good idea!]

Done, and renamed some tags both to avoid duplicating the group name and for consistency.

nilsdeppe · 2023-12-07T18:15:58Z

src/Evolution/Imex/Tags/SolveTolerance.hpp

+
+namespace imex {
+namespace OptionTags {
+struct ImexImplicitSolveTolerance {


same doxygen question

nilsdeppe · 2023-12-09T03:15:14Z

src/Evolution/Imex/SolveImplicitSector.hpp

+  using attempt_tags = tmpl::transform<typename ImplicitSector::solve_attempts,
+                                       get_tags_from_evolution<tmpl::_1>>;
+  using evolution_data_tags =
+      tmpl::push_front<attempt_tags,


Is it possible tmpl::append here to combine the two lists? If not, maybe a comment why?

Renamed a thing and added some comments to try to make this clearer. Not sure if it's enough.

nilsdeppe · 2023-12-09T03:19:47Z

src/Evolution/Imex/SolveImplicitSector.tpp

+  // Only allocated if used.
+  Matrix semi_implicit_jacobian{};
+
+  bool had_failure = true;


I think I would find reading the code easier if this was solve_succeeded or something like that. not had_failure I think is success and I would find it easier to read

if (solve_succeeded) { return; }

nilsdeppe · 2023-12-09T16:36:45Z

src/Evolution/Imex/SolveImplicitSector.tpp

+    return inhomogeneous_terms_;
+  }
+
+  bool do_solve() const { return implicit_weight_ != 0.0; }


the function name makes me think it does some computation. Maybe rename to something like solved_needed?

nilsdeppe · 2023-12-09T16:46:08Z

src/Evolution/Imex/SolveImplicitSector.tpp

+      : solve_box_(db::create<simple_tags, compute_tags>()),
+        implicit_equation_(&implicit_equation) {
+    static_assert(std::is_same_v<PassedEvolutionData, const EvolutionData&>,
+                  "ImplicitSolver was passed a temporary.");


Could you expand this error message? I'm not sure what I should do if I were to get this.

You should stop passing a temporary. I've added a brief explanation of why.

nilsdeppe · 2023-12-09T17:03:56Z

src/Evolution/Imex/SolveImplicitSector.tpp

+};
+
+template <typename ImplicitSector, typename SolveAttempt>
+class ImplicitSolver {


Would some overview documentation be useful here? Not necessarily for doxygen, but to get a quick overview of what the different functions do and why? Maybe also for ImplicitEquation?

nilsdeppe · 2023-12-09T17:18:34Z

tests/Unit/Helpers/Evolution/Imex/TestSector.hpp

+struct ReadOnly : Tag, ::db::ComputeTag {
+  using base = Tag;
+  using argument_tags = tmpl::list<ReadOnlySource<Tag>>;
+  static void function(gsl::not_null<typename ReadOnly::type*> dest,


const gsl::not_null?

nilsdeppe · 2023-12-11T15:24:11Z

tests/Unit/Evolution/Imex/Test_SolveImplicitSector.cpp

+};
+// End stuff only used for DataBox handling
+
+// [initial_guess]


Do you want this initial_guess to be around the AnalyticSolution or the InitialGuess class below? Right now I think it's around the AnalyticSolution

This is intentional. They are both valid "initial_guess" structs (the example chooses between them with a tmpl::conditional_t), but this one does a reasonable calculation while InitialGuess just does some nonsensical modifications to the data.

wthrowe

This is now dependent on (and includes the commits from) #5657.

wthrowe · 2023-12-12T23:29:05Z

src/Evolution/Imex/SolveImplicitSector.hpp

+  using attempt_tags = tmpl::transform<typename ImplicitSector::solve_attempts,
+                                       get_tags_from_evolution<tmpl::_1>>;
+  using evolution_data_tags =
+      tmpl::push_front<attempt_tags,


Renamed a thing and added some comments to try to make this clearer. Not sure if it's enough.

wthrowe · 2023-12-12T23:32:23Z

src/Evolution/Imex/SolveImplicitSector.tpp

+      : solve_box_(db::create<simple_tags, compute_tags>()),
+        implicit_equation_(&implicit_equation) {
+    static_assert(std::is_same_v<PassedEvolutionData, const EvolutionData&>,
+                  "ImplicitSolver was passed a temporary.");


You should stop passing a temporary. I've added a brief explanation of why.

wthrowe · 2023-12-12T23:36:17Z

src/Evolution/Imex/SolveImplicitSector.tpp

+          break;
+        }
+        case Mode::SemiImplicit: {
+          auto correction_array = solver(initial_guess);


There weren't supposed to be any after the first point, but you're right that I missed one in the LAPACK wrapper, so I fixed that.

I've made the types of some variables and temporaries explicitly arrays, so it should be clear that there are no allocations in them.

wthrowe · 2023-12-13T01:43:53Z

tests/Unit/Evolution/Imex/Test_SolveImplicitSector.cpp

+};
+// End stuff only used for DataBox handling
+
+// [initial_guess]


This is intentional. They are both valid "initial_guess" structs (the example chooses between them with a tmpl::conditional_t), but this one does a reasonable calculation while InitialGuess just does some nonsensical modifications to the data.

wthrowe · 2023-12-13T01:49:23Z

src/Evolution/Imex/Tags/Mode.hpp

+struct ImexMode {
+  static constexpr Options::String help{"IMEX implementation to use"};
+  using type = ::imex::Mode;
+  using group = evolution::OptionTags::Group;


Done, and renamed some tags both to avoid duplicating the group name and for consistency.

nilsdeppe

LGTM, please go ahead and squash!

wthrowe · 2023-12-15T03:15:55Z

@nilsdeppe I think this is ready to go.

wthrowe force-pushed the imex/solve_implicit_sector branch 2 times, most recently from 0e38ae4 to adbe671 Compare December 6, 2023 04:19

nilsdeppe requested changes Dec 11, 2023

View reviewed changes

wthrowe force-pushed the imex/solve_implicit_sector branch from adbe671 to 95395bc Compare December 13, 2023 21:33

wthrowe commented Dec 13, 2023

View reviewed changes

nilsdeppe reviewed Dec 14, 2023

View reviewed changes

wthrowe added 4 commits December 14, 2023 17:04

Add IMEX Mode enum

ab939d1

Add IMEX tags for data

f01d347

Add IMEX tags for options

657d2f0

Add IMEX SolveImplicitSector mutator

da76af4

wthrowe force-pushed the imex/solve_implicit_sector branch from 95395bc to da76af4 Compare December 14, 2023 22:56

nilsdeppe approved these changes Dec 15, 2023

View reviewed changes

nilsdeppe merged commit 71cf42d into sxs-collaboration:develop Dec 15, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IMEX SolveImplicitSector mutator #5653

Add IMEX SolveImplicitSector mutator #5653

wthrowe commented Dec 5, 2023

nilsdeppe left a comment

nilsdeppe Dec 7, 2023

nilsdeppe Dec 7, 2023

wthrowe Dec 13, 2023

nilsdeppe Dec 7, 2023

nilsdeppe Dec 9, 2023

wthrowe Dec 12, 2023

nilsdeppe Dec 9, 2023

nilsdeppe Dec 9, 2023

nilsdeppe Dec 9, 2023

wthrowe Dec 12, 2023

nilsdeppe Dec 9, 2023

nilsdeppe Dec 9, 2023

nilsdeppe Dec 11, 2023

wthrowe Dec 13, 2023

wthrowe left a comment

wthrowe Dec 12, 2023

wthrowe Dec 12, 2023

wthrowe Dec 12, 2023

wthrowe Dec 13, 2023

wthrowe Dec 13, 2023

nilsdeppe left a comment

wthrowe commented Dec 15, 2023

Add IMEX SolveImplicitSector mutator #5653

Add IMEX SolveImplicitSector mutator #5653

Conversation

wthrowe commented Dec 5, 2023

Proposed changes

Upgrade instructions

Code review checklist

Further comments

nilsdeppe 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

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

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

nilsdeppe left a comment

Choose a reason for hiding this comment

wthrowe commented Dec 15, 2023