Vector-ordered Rosenbrock Solver #172

boulderdaze · 2024-07-03T20:38:59Z

Closes #55

Update musica build options
Doesn't support multi grid cells and vector matrix size greater than 1. (the way assigning the concentrations in Solve function)
I decided to submit this with the limitation described above because this task currently blocks implementing Backward Euler as well as a separate issue was created to support multiple grid cells.
These comments are not valid anymore but left for record to describe where the bug found.
Added compilation flag for the number of grid cells: MUSICA_SET_NUM_GRID_CELLS
For vector-ordered matrix solver, when number of grid cell is less than L value, for example, num_grid_cell=1 , L=4 , num_species=5 , it fails during solving because the Copy() call throws an error indicating that the dimension is not the same; copying other vector to this vector (5 /= 20). Since L is decided at compile time, checks should be included in the build script

mattldawson

looks good! just a couple questions and suggestions. the only important one is that that the number of grid cells won't be known at compile time

CMakeLists.txt

fortran/test/fetch_content_integration/test_micm_api.F90

include/musica/micm.hpp

src/test/unit/micm/micm_c_api.cpp

boulderdaze · 2024-07-09T23:18:59Z

cmake/test_util.cmake

  if(MUSICA_ENABLE_OPENMP)
-    target_link_libraries(test_${TEST_NAME} PUBLIC OpenMP::OpenMP_CXX OpenMP::OpenMP_Fortran)
+    target_link_libraries(test_${TEST_NAME} PUBLIC OpenMP::OpenMP_CXX)
  endif()


Hi @mattldawson and @K20shores, what do you think about this change? Matt and I briefly discussed about this. The issue is that OpenMP::OpenMP_Fortran is not found when test_micm_c_api is built. I removed it because this line belongs to function(create_standard_test_cxx) and I assumed that it shouldn't need OpenMp_Fortran for c++ tests.
Matt mentioned we could specify fortran as one of the languages in the top-level CMakeLists.txt and update find_package in cmake/dependencies.cmake

project(musica-distribution VERSION 0.7.0 LANGUAGES C CXX Fortran) find_package(OpenMP REQUIRED COMPONENTS C CXX Fortran)

I think that makes sense although I think it is useful to separate the fortran stuff from MUSICA-C as it is since we eventually pursue fortran-free. I'd like to hear what you all think.

I would like to keep fortran separated, but with the inclusion of tuvx into the musica-c library that line is blurred. Because of that I think we should go with Matt's suggestion. The other option is to check if TUVX is enabled before we check for OpenMP. If it is, enable the fortran language so that the fortran part of OpenMP is found, if that's possible

I created a separate issue to tackle this.

src/test/unit/micm/micm_c_api.cpp

K20shores

I think it all looks fine as is, but we could make it slightly better. I wonder if we could remove the need to have two member variable pointers to different solver types and two different build functions when only the matrix types are different

K20shores · 2024-07-11T14:20:14Z

fortran/micm.F90

+   ! We could use Fortran 2023 enum type feature if Fortran 2023 is supported
+   ! https://fortran-lang.discourse.group/t/enumerator-type-in-bind-c-derived-type-best-practice/5947/2
+   enum, bind(c)
+      enumerator :: Rosenbrock              = 1
+      enumerator :: RosenbrockStandardOrder = 2
+   end enum


As long as our CI tests pass then I'm fine with this. The only potential issue maybe is the NAG compiler and if it supports this and if we need to compile with NAG ever.

But I like this as it is

Jesse mentioned that it's probably not yet to use Fortran 2023 features but I'll leave the comment so we can revisit later.

I like this too

K20shores · 2024-07-11T14:24:37Z

include/musica/micm.hpp

+    /// @brief Vector-ordered Rosenbrock solver type
+    using DenseMatrixVector = micm::VectorMatrix<double, MICM_VECTOR_MATRIX_SIZE>;
+    using SparseMatrixVector = micm::SparseMatrix<double, micm::SparseMatrixVectorOrdering<MICM_VECTOR_MATRIX_SIZE>>;
+    using RosenbrockVectorType = typename micm::RosenbrockSolverParameters::
+        template SolverType<micm::ProcessSet, micm::LinearSolver<SparseMatrixVector, micm::LuDecomposition>>;
+    using Rosenbrock = micm::Solver<RosenbrockVectorType, micm::State<DenseMatrixVector, SparseMatrixVector>>;
+    std::unique_ptr<Rosenbrock> rosenbrock_;

-    std::unique_ptr<Rosenbrock> solver_;
+    /// @brief Standard-ordered Rosenbrock solver type
+    using DenseMatrixStandard = micm::Matrix<double>;
+    using SparseMatrixStandard = micm::SparseMatrix<double, micm::SparseMatrixStandardOrdering>;
+    using RosenbrockStandardType = typename micm::RosenbrockSolverParameters::
+        template SolverType<micm::ProcessSet, micm::LinearSolver<SparseMatrixStandard, micm::LuDecomposition>>;
+    using RosenbrockStandard = micm::Solver<RosenbrockStandardType, micm::State<DenseMatrixStandard, SparseMatrixStandard>>;
+    std::unique_ptr<RosenbrockStandard> rosenbrock_standard_;


Are we allowed to have a pointer to a micm::Solver without specifying the template types?

or maybe we use std::variant?

Do you mean something like std::unique_ptr<micm::Solver> solver? If it is the case, I tried it but it didn't work, (template argument is invalid). UnlessMICM is a template class which is not a viable option, I think all the types must be specified.

I tried implementing std::variant and realized that you still need to do type check to get the data that variant holds, and the syntax can be quite tricky. For example, MICM::Solve( lots of params ... )

// Current micm::State state = solver->GetState(); // std::variant if(std::get_if<Rosenbrock>(&solver)) micm::State state = std::get<Rosenbrock>(&solver)->GetState(); if(std::get_if<RosenbrockStandardOrder>(&solver)) micm::State state = std::get<RosenbrockStandardOrder>(&solver)->GetState();

What do you think? The fact that C doesn't support any form of overloading, we quite have limitations here.

Hmm. Good point. What you have seems fine anyways

K20shores · 2024-07-11T14:29:05Z

src/micm/micm.cpp

+          micm::SolverBuilder<
+              micm::RosenbrockSolverParameters,
+              micm::VectorMatrix<double, MICM_VECTOR_MATRIX_SIZE>,
+              micm::SparseMatrix<double, micm::SparseMatrixVectorOrdering<MICM_VECTOR_MATRIX_SIZE>>,
+              micm::ProcessSet,
+              micm::LinearSolver<
+                  micm::SparseMatrix<double, micm::SparseMatrixVectorOrdering<MICM_VECTOR_MATRIX_SIZE>>,
+                  micm::LuDecomposition>>(micm::RosenbrockSolverParameters::ThreeStageRosenbrockParameters())


maybe we should make something similar to micm::CpuSolverBuilder but make it be micm::VectorizedCpuSolverBuilder which is templated on the on vector size. Then it would be able to do all of this gross stuff for us. As an added benefit we should be able to add some alias types inside all the solver builder aliases that give us the type of the solver so that inside the header file we would only need to do something like

using builderType.= micm::CpuSolverBuilder; using solverType = builderType::RosenbrockType;

I like this idea. I created an issue for this in MICM. I think this can be a separate issue because alias-declaration for builder type should be done first at MICM.

mattldawson

looks great!

boulderdaze added 17 commits June 24, 2024 14:31

update cmake options

4ac0fab

set vector matrix dimension

e27634b

compilation flag for grid cells, vector matrix dimension

d114885

c api for vector ordered matrix

6d2bc7d

add vector matrix test

bf57538

update fortran api

090b620

merge main

062c748

chage the build type

4546e4a

update python wrapper

fa82486

resolve memory leak

54ec3e7

fix intel docker

b362330

tuvx on

30bfafc

micm git tag

4c10839

edit erorr

b484cfe

opdate the option name for open mp

2d50377

remove open mp fortran in c tests

f4f6526

edit comment

b8481f6

boulderdaze marked this pull request as ready for review July 5, 2024 22:42

boulderdaze requested review from mattldawson and K20shores July 5, 2024 22:43

mattldawson requested changes Jul 8, 2024

View reviewed changes

boulderdaze added 2 commits July 8, 2024 16:03

replace unique ptr T to T

5a67815

replace tempalte with auto

983a510

boulderdaze marked this pull request as draft July 9, 2024 19:29

boulderdaze added 5 commits July 9, 2024 14:15

add map header

997038b

Merge branch 'main' into 55-vector-ordered-rosenbrock

8b08448

expose solver type enum to python

80a0f66

back to template

23889c3

back to template

276bc8f

boulderdaze commented Jul 9, 2024

View reviewed changes

boulderdaze added 5 commits July 9, 2024 18:23

enum for solver type

2812b21

enum solver type for fortran

09bf9b8

add fortran 2023 feature comment

680a2cb

pass num grid cells to constructor

61c87df

code clean up

63bf319

boulderdaze marked this pull request as ready for review July 10, 2024 21:28

boulderdaze requested a review from mattldawson July 10, 2024 21:29

boulderdaze mentioned this pull request Jul 10, 2024

Uniformize the indentation for Fortran code base #177

Merged

K20shores approved these changes Jul 11, 2024

View reviewed changes

boulderdaze added 4 commits July 11, 2024 13:36

specify enabled languages in top level cmake

15ae988

comment out the tuvx openmp tests

e53faf2

revert back

1f548de

revert back

830b426

mattldawson approved these changes Jul 12, 2024

View reviewed changes

K20shores approved these changes Jul 15, 2024

View reviewed changes

boulderdaze merged commit 2fff1fb into main Jul 15, 2024
61 checks passed

boulderdaze deleted the 55-vector-ordered-rosenbrock branch July 15, 2024 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vector-ordered Rosenbrock Solver #172

Vector-ordered Rosenbrock Solver #172

boulderdaze commented Jul 3, 2024 •

edited

Loading

mattldawson left a comment

boulderdaze Jul 9, 2024

K20shores Jul 11, 2024

boulderdaze Jul 11, 2024

K20shores left a comment

K20shores Jul 11, 2024

boulderdaze Jul 11, 2024

mattldawson Jul 12, 2024

K20shores Jul 11, 2024

K20shores Jul 11, 2024

boulderdaze Jul 11, 2024 •

edited

Loading

K20shores Jul 15, 2024

K20shores Jul 11, 2024

boulderdaze Jul 11, 2024 •

edited

Loading

mattldawson left a comment

Vector-ordered Rosenbrock Solver #172

Vector-ordered Rosenbrock Solver #172

Conversation

boulderdaze commented Jul 3, 2024 • edited Loading

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

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

boulderdaze Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boulderdaze Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

mattldawson left a comment

Choose a reason for hiding this comment

boulderdaze commented Jul 3, 2024 •

edited

Loading

boulderdaze Jul 11, 2024 •

edited

Loading

boulderdaze Jul 11, 2024 •

edited

Loading