Change LU decomposition class to be Doolittle-specific #683

sjsprecious · 2024-11-08T22:36:12Z

This PR renames the LU decomposition class to Doolittle, which is the specific implementation for LU decomposition in MICM.

fix #682

codecov-commenter · 2024-11-08T23:49:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.19%. Comparing base (3f0c607) to head (b0ca55d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #683   +/-   ##
=======================================
  Coverage   93.19%   93.19%           
=======================================
  Files          53       53           
  Lines        3586     3586           
=======================================
  Hits         3342     3342           
  Misses        244      244

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

K20shores

I have a preference for only adding doolittle to the LU decomposition class and either leaving the solver builder names alone or renaming them to Default*. But I can be convinced either way

K20shores · 2024-11-11T14:15:30Z

include/micm/jit/solver/jit_solver_builder.hpp

@@ -17,7 +17,7 @@ namespace micm
  ///
  /// JIT-compiled solvers only work with vector-ordered matrices
  template<class SolverParametersPolicy, std::size_t L = MICM_DEFAULT_VECTOR_SIZE>
-  using JitSolverBuilder = SolverBuilder<
+  using JitSolverBuilder_Doolittle = SolverBuilder<


Suggested change

using JitSolverBuilder_Doolittle = SolverBuilder<

using DefaultJITSolverBuilder = SolverBuilder<

I prefer this instead of adding doolittle to the name. What do you think?

Thanks Kyle. I do not like my current name change to the solver builder either. If I add the Default prefix to the solver builder as you suggested, what would be the alternative name when we add the new LU decomposition implementation?

If it's faster, maybe _Fast? That'll at least indicate that something fancy is going on

That sounds great. For consistency purpose, is it better to put both Default and Fast as prefix or suffix?

Personally, I would just leave this as JitSolverBuilder. If the MOZART algorithm is faster, but someone wants to use the Doolittle algorithm (for some reason), they can just specify all the template parameters for the solver builder themselves

Thanks @mattldawson . I just revert to the original builder name and we can do what you suggest after we implement the MOZART algorithm.

K20shores · 2024-11-11T14:15:50Z

include/micm/cuda/solver/cuda_solver_builder.hpp

@@ -21,7 +21,7 @@ namespace micm
  ///
  /// GPU solvers only work with vector-ordered matrices
  template<class SolverParametersPolicy, std::size_t L = MICM_DEFAULT_VECTOR_SIZE>
-  using CudaSolverBuilder = SolverBuilder<
+  using CudaSolverBuilder_Doolittle = SolverBuilder<


Suggested change

using CudaSolverBuilder_Doolittle = SolverBuilder<

using DefaultCUDASolverBuilder = SolverBuilder<

I prefer this instead of adding doolittle to the name. What do you think?

As Matt suggested, we may just keep the original name for now.

K20shores · 2024-11-11T14:16:29Z

examples/example.cpp

@@ -47,7 +47,7 @@ int main(const int argc, const char* argv[])
  auto chemical_system = solver_params.system_;
  auto reactions = solver_params.processes_;

-  auto solver = CpuSolverBuilder<micm::RosenbrockSolverParameters>(solver_params.parameters_)
+  auto solver = CpuSolverBuilder_DoolittleLU<micm::RosenbrockSolverParameters>(solver_params.parameters_)


Suggested change

auto solver = CpuSolverBuilder_DoolittleLU<micm::RosenbrockSolverParameters>(solver_params.parameters_)

auto solver = DefaultCPUSolverBuilder<micm::RosenbrockSolverParameters>(solver_params.parameters_)

I prefer this instead of adding doolittle to the name. What do you think?

agreed, or just leave it as CpuSolverBuilder. If the MOZART LU algorithm is faster, we'll probably just use that in the examples and tutorials

As Matt suggested, we may just keep the original name for now.

mattldawson

looks good! Just a couple comments related to the alias names

mattldawson · 2024-11-11T21:54:10Z

examples/example.cpp

@@ -47,7 +47,7 @@ int main(const int argc, const char* argv[])
  auto chemical_system = solver_params.system_;
  auto reactions = solver_params.processes_;

-  auto solver = CpuSolverBuilder<micm::RosenbrockSolverParameters>(solver_params.parameters_)
+  auto solver = CpuSolverBuilder_DoolittleLU<micm::RosenbrockSolverParameters>(solver_params.parameters_)


agreed, or just leave it as CpuSolverBuilder. If the MOZART LU algorithm is faster, we'll probably just use that in the examples and tutorials

mattldawson · 2024-11-11T21:54:29Z

examples/profile_example.cpp

@@ -34,7 +34,7 @@ namespace fs = std::filesystem;
 using namespace micm;

 template<std::size_t L>
-using VectorBuilder = CpuSolverBuilder<RosenbrockSolverParameters, VectorMatrix<double, L>, SparseMatrix<double, SparseMatrixVectorOrdering<L>>>;
+using VectorBuilder = CpuSolverBuilder_DoolittleLU<RosenbrockSolverParameters, VectorMatrix<double, L>, SparseMatrix<double, SparseMatrixVectorOrdering<L>>>;


same as above

mattldawson · 2024-11-11T21:57:04Z

include/micm/jit/solver/jit_solver_builder.hpp

@@ -17,7 +17,7 @@ namespace micm
  ///
  /// JIT-compiled solvers only work with vector-ordered matrices
  template<class SolverParametersPolicy, std::size_t L = MICM_DEFAULT_VECTOR_SIZE>
-  using JitSolverBuilder = SolverBuilder<
+  using JitSolverBuilder_Doolittle = SolverBuilder<


Personally, I would just leave this as JitSolverBuilder. If the MOZART algorithm is faster, but someone wants to use the Doolittle algorithm (for some reason), they can just specify all the template parameters for the solver builder themselves

sjsprecious · 2024-11-11T22:21:19Z

Thanks @mattldawson . I just revert the builder to use its original name as suggested.

mattldawson

nice!

change LU decomposition class to be Doolittle specific

9585983

sjsprecious requested review from mattldawson and K20shores November 8, 2024 22:36

sjsprecious self-assigned this Nov 8, 2024

sjsprecious added the enhancement New feature or request label Nov 8, 2024

sjsprecious added this to the MICM Paper milestone Nov 8, 2024

sjsprecious added 5 commits November 8, 2024 15:42

fix JIT compilation bug

88acda8

hope this is the last missing one

01fdf0b

fix the jit lu deposition file

a9704cf

maybe this is the last one

00a8b47

change builder name for CUDA and JIT

8f69820

K20shores approved these changes Nov 11, 2024

View reviewed changes

mattldawson requested changes Nov 11, 2024

View reviewed changes

revert to use the original builder name

b0ca55d

mattldawson approved these changes Nov 11, 2024

View reviewed changes

sjsprecious merged commit 21a6d64 into main Nov 11, 2024
29 checks passed

sjsprecious deleted the rename_lu_decomp_class branch November 11, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change LU decomposition class to be Doolittle-specific #683

Change LU decomposition class to be Doolittle-specific #683

sjsprecious commented Nov 8, 2024

codecov-commenter commented Nov 8, 2024 •

edited

Loading

K20shores left a comment

K20shores Nov 11, 2024

sjsprecious Nov 11, 2024

K20shores Nov 11, 2024

sjsprecious Nov 11, 2024

mattldawson Nov 11, 2024

sjsprecious Nov 11, 2024

K20shores Nov 11, 2024

sjsprecious Nov 11, 2024

K20shores Nov 11, 2024

mattldawson Nov 11, 2024

sjsprecious Nov 11, 2024

mattldawson left a comment

mattldawson Nov 11, 2024

mattldawson Nov 11, 2024

mattldawson Nov 11, 2024

sjsprecious commented Nov 11, 2024

mattldawson left a comment

	using JitSolverBuilder_Doolittle = SolverBuilder<
	using DefaultJITSolverBuilder = SolverBuilder<

	using CudaSolverBuilder_Doolittle = SolverBuilder<
	using DefaultCUDASolverBuilder = SolverBuilder<

	auto solver = CpuSolverBuilder_DoolittleLU<micm::RosenbrockSolverParameters>(solver_params.parameters_)
	auto solver = DefaultCPUSolverBuilder<micm::RosenbrockSolverParameters>(solver_params.parameters_)

Change LU decomposition class to be Doolittle-specific #683

Change LU decomposition class to be Doolittle-specific #683

Conversation

sjsprecious commented Nov 8, 2024

codecov-commenter commented Nov 8, 2024 • edited Loading

Codecov Report

K20shores 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

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

Choose a reason for hiding this comment

sjsprecious commented Nov 11, 2024

mattldawson left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 8, 2024 •

edited

Loading