162 euler option #197
162 euler option #197
Conversation
There was a problem hiding this comment.
Looks good! I made a couple suggestions and opened discussion. I understand that you wanted to open a separate PR for fortran and python APIs although I think it could possibly break the other APIs that we don't know. (for example enum MICMSolver) I think it might be a good idea to have them all in one PR so we make sure that the changes are all applied and fully tested across the different language APIs. I'd like to hear what you all think
| Rosenbrock = 1, // Vector-ordered Rosenbrock solver | ||
| BackwardEuler, // Vector-ordered BackwardEuler solver | ||
| RosenbrockStandardOrder, // Standard-ordered Rosenbrock solver | ||
| BackwardEulerStandardOrder, // Standard-ordered BackwardEuler solver | ||
| }; |
There was a problem hiding this comment.
The change of the enum order will fail the fortran micm APIs. I guess the ordering based on the matrix type is fine as it is because we only have two types of solver, but if we think about adding more solvers, it'd probably be easier for maintainers to order them based on the solver type, then the enum value doesn't have to change every time we add a new solver. Also for the users, if a new release has to change the ordering, it could be confusing.
There was a problem hiding this comment.
I agree it will better to order by solver type / matrix type ... will also rearrange source with this ordering.
| @@ -140,11 +142,21 @@ namespace musica | |||
| /// @param error Error struct to indicate success or failure | |||
| void CreateRosenbrock(const std::string &config_path, Error *error); | |||
|
|
|||
| /// @brief Create a BackwardEuler solver of vector-ordered matrix type by reading and parsing configuration file | |||
There was a problem hiding this comment.
I have the same thoughts on the ordering of the functions related to the above comments.
| // Test case for solving system using vector-ordered BackwardEuler solver | ||
| TEST_F(MicmCApiTest, SolveUsingVectorOrderedBackwardEuler) | ||
| { | ||
| const char* config_path = "configs/chapman"; | ||
| int num_grid_cells = 1; | ||
| Error error; | ||
|
|
||
| MICM* micm = CreateMicm(config_path, MICMSolver::BackwardEuler, num_grid_cells, &error); | ||
|
|
||
| double time_step = 200.0; | ||
| double temperature = 272.5; | ||
| double pressure = 101253.3; | ||
| constexpr double GAS_CONSTANT = 8.31446261815324; // J mol-1 K-1 | ||
| double air_density = pressure / (GAS_CONSTANT * temperature); | ||
| int num_concentrations = 4; | ||
| double concentrations[] = { 0.4, 0.8, 0.01, 0.02 }; | ||
| std::size_t num_user_defined_reaction_rates = 3; | ||
| double user_defined_reaction_rates[] = { 0.1, 0.2, 0.3 }; | ||
| String solver_state; | ||
| SolverResultStats solver_stats; | ||
|
|
||
| Mapping* ordering = GetUserDefinedReactionRatesOrdering(micm, &num_user_defined_reaction_rates, &error); | ||
| ASSERT_TRUE(IsSuccess(error)); | ||
|
|
||
| ASSERT_EQ(num_user_defined_reaction_rates, 3); | ||
|
|
||
| std::vector<double> custom_rate_parameters(num_user_defined_reaction_rates, 0.0); | ||
| for (std::size_t i = 0; i < num_user_defined_reaction_rates; i++) | ||
| { | ||
| custom_rate_parameters[ordering[i].index_] = 0.0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Matt's open PR #192 for adding mutiple grid cells brings some big changes in micm api test. I think the tests for backward euler has to be align with what he set up. Could you review his PR and make adjustment on the test for backward euler?
There was a problem hiding this comment.
Looks great! I agree with @boulderdaze about updating the tests if you could to follow the logic of the modified Rosenbrock tests. (Hopefully, you're able to use the same functions as for the Rosenbrock tests and just pass them a Backward Euler solver)
|
Reissuing PR from new branch. |
Added C API options for the Backward Euler Solver, with vector ordered or standard ordered matrices.
Unit tests follow those for the Rosenbrock C API.