514 create a builder class that can make a backward euler solver #527

K20shores · 2024-05-17T17:28:58Z

Closes #514

Temporarily removes backward euler from running in the analytical policy test until we finish implementing the builder such that any solver could be run with those tests
Adds a solver builder, which has options to build different solvers, right now only BackwardEuler is supported
Adds a solver class which wraps any solver (backward euler, rosenbrock). This may be a useless class
Removes template template parameters from many different places

sjsprecious

Thanks @K20shores for working on this giant PR. Really nice work and I just have a few clarification questions besides the three failing tests.

sjsprecious · 2024-05-21T20:13:22Z

test/unit/solver/test_cuda_linear_solver.cpp

-        return micm::LinearSolver<double, Group10000SparseVectorMatrix>{ matrix, initial_value };
-      },
-      10000);
+      micm::LinearSolver<Group10000SparseVectorMatrix>>(10000);


why don't we need the lambda function here any more?

It's not clear to me that we ever needed it here. we can call the construction directly in the linear solver function. I can put these back if we like having the lamdas with explicit creation

Thanks. @mattldawson do you recall why we use the lambda function argument before?

It was for the JITed version of things. We needed to pass the jit compiler around, but, I made the jit compiler into a singleton and now the interface to all solvers and their components are the same

test/unit/solver/test_cuda_lu_decomposition.cpp

boulderdaze

Looks good! Just have minor change requests. I like the removal of template template parameter.

include/micm/solver/solver_builder.hpp

include/micm/solver/backward_euler.hpp

include/micm/util/matrix.hpp

codecov-commenter · 2024-05-23T18:54:01Z

Codecov Report

Attention: Patch coverage is 85.99222% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 92.25%. Comparing base (5afd404) to head (a0aa13e).
Report is 1 commits behind head on main.

Files	Patch %	Lines
include/micm/solver/solver_builder.inl	71.92%	32 Missing ⚠️
include/micm/jit/jit_compiler.hpp	91.89%	3 Missing ⚠️
include/micm/solver/jit_linear_solver.inl	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
- Coverage   92.90%   92.25%   -0.65%     
==========================================
  Files          42       45       +3     
  Lines        3268     3408     +140     
==========================================
+ Hits         3036     3144     +108     
- Misses        232      264      +32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

boulderdaze

That makes sense that the Jit class is singleton. Looks cleaner!

mattldawson

looks great! this was a lot of work! Just a couple minor questions, but nothing important

include/micm/jit/jit_function.hpp

mattldawson · 2024-05-24T17:15:32Z

include/micm/solver/solver_builder.inl

+  template<class DenseMatrixPolicy, class SparseMatrixPolicy>
+  inline auto SolverBuilder<DenseMatrixPolicy, SparseMatrixPolicy>::Build()
+  {
+    if (std::holds_alternative<RosenbrockSolverParameters>(options_))
+    {
+      throw std::runtime_error("Not implemented yet");
+    }
+    if (std::holds_alternative<BackwardEulerSolverParameters>(options_))
+    {
+      return BuildBackwardEulerSolver();
+    }
+
+    throw std::runtime_error("No solver type set");
+  }


For the ProcessBuilder we have a Process(ProcessBuilder&) constructor instead of a ProcessBuilder::Build() function. Would it make sense to use the same approach here?

I don't have a preference on if we do that or not. Maybe you could do that in the next PR? This one already has so much

test/tutorial/test_README_example.cpp

K20shores · 2024-05-24T18:46:54Z

Intel failure seems to be in other repositories as well

K20shores · 2024-05-24T18:47:31Z

Tracking the intel issue: https://community.intel.com/t5/Intel-oneAPI-Math-Kernel-Library/Cannot-update-any-packages-using-intel-oneapi-hpckit-latest/td-p/1600608

sjsprecious

Thanks @K20shores for working on this giant PR.

All the 44 tests pass on Derecho's A100 GPU, using nvhpc/23.7.

mwaxmonsky

Great job @K20shores!

Just had a few comments/questions but nothing that should hold this up.

include/micm/solver/backward_euler_solver_parameters.hpp

mwaxmonsky · 2024-05-29T13:41:40Z

test/integration/analytical_policy.hpp

+    // if constexpr (std::is_same_v<decltype(solver.process_set_), micm::ProcessSet>)
+    // {
+    //   auto linear_solver = solver.linear_solver_;
+    //   auto process_set = solver.process_set_;
+    //   be.Solve(
+    //     time_step, be_state, micm::BackwardEulerSolverParameters(), linear_solver, process_set, processes, solver.state_parameters_.jacobian_diagonal_elements_);
+    // }


Can we delete this now or is this going to be re-used in the future?

Oh, I do think we can delete this. I'm hoping that all of these tests can be passed any type of solver and check the results. I can dream and hope to make it come true by deleting those

test/integration/analytical_policy.hpp

test/unit/solver/test_solver_builder.cpp

K20shores and others added 8 commits May 13, 2024 16:47

starting to piece together the builder

b52a86d

hiding some template details behind a solver impl

8002c87

removing a lot of template template parameters

fe22e55

removing more template templates, compiling with gcc

ae78706

chaning function name and adding a type alias

eb2c6d8

adding state policy in progress

42de6d9

update for new state policies

100b3d6

getting a solver builder to work

59fc99a

K20shores requested review from sjsprecious, mattldawson and boulderdaze May 17, 2024 17:28

K20shores linked an issue May 17, 2024 that may be closed by this pull request

Create a builder class that can make a Backward euler solver #514

Closed

K20shores requested a review from mwaxmonsky May 17, 2024 17:29

K20shores added 3 commits May 17, 2024 13:38

it compiles with clang on my machine

82a32be

making the dockerfile build

af79b76

making sure profiling option compiles

c0782ef

sjsprecious assigned K20shores May 17, 2024

sjsprecious added the enhancement New feature or request label May 17, 2024

sjsprecious added this to the MICM in CAM-SIMA milestone May 17, 2024

K20shores added 2 commits May 20, 2024 12:22

attempting to get cuda to compile

bdfcf91

i think nvidia all compiles now

4d03171

sjsprecious reviewed May 21, 2024

View reviewed changes

boulderdaze requested changes May 21, 2024

View reviewed changes

include/micm/solver/solver_builder.hpp Outdated Show resolved Hide resolved

include/micm/solver/backward_euler.hpp Outdated Show resolved Hide resolved

include/micm/util/matrix.hpp Show resolved Hide resolved

K20shores added 5 commits May 23, 2024 09:14

starting to make the jit compiler a singleton

2ac73fa

trying to pull my old changes

8497e9c

trying to define jit compiler as a singleton

e15bdd3

saving progress just in case since I've made some

a4e7d79

it compiles

2fb3917

removing more lambdas

c93c87f

K20shores added 5 commits May 23, 2024 16:40

suppresing openmp checks

15bd1f8

updating suppresion file

b678291

addressing PR comments

b4358b7

correcting implementation

4557b58

merging main

49d9e5f

boulderdaze approved these changes May 24, 2024

View reviewed changes

mattldawson approved these changes May 24, 2024

View reviewed changes

reverting readme example

63368c0

sjsprecious approved these changes May 24, 2024

View reviewed changes

mwaxmonsky approved these changes May 29, 2024

View reviewed changes

addressing PR comments

a0aa13e

mofosyne mentioned this pull request May 29, 2024

intel/oneapi-basekit APT Repository not working intel/oneapi-containers#70

Closed

K20shores merged commit 369aede into main May 29, 2024
44 of 45 checks passed

K20shores deleted the 514-create-a-builder-class-that-can-make-a-backward-euler-solver branch May 29, 2024 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

514 create a builder class that can make a backward euler solver #527

514 create a builder class that can make a backward euler solver #527

K20shores commented May 17, 2024

sjsprecious left a comment

sjsprecious May 21, 2024

K20shores May 21, 2024

sjsprecious May 21, 2024

K20shores May 24, 2024

boulderdaze left a comment

codecov-commenter commented May 23, 2024 •

edited

Loading

boulderdaze left a comment

mattldawson left a comment

mattldawson May 24, 2024

K20shores May 28, 2024

K20shores commented May 24, 2024

K20shores commented May 24, 2024

sjsprecious left a comment

mwaxmonsky left a comment

mwaxmonsky May 29, 2024

K20shores May 29, 2024

514 create a builder class that can make a backward euler solver #527

514 create a builder class that can make a backward euler solver #527

Conversation

K20shores commented May 17, 2024

sjsprecious 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

boulderdaze left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 23, 2024 • edited Loading

Codecov Report

boulderdaze left a comment

Choose a reason for hiding this comment

mattldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K20shores commented May 24, 2024

K20shores commented May 24, 2024

sjsprecious left a comment

Choose a reason for hiding this comment

mwaxmonsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 23, 2024 •

edited

Loading