MUSICA C API Backward Euler options #193
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.
| /// @brief Create a BackwardEuler solver of vector-ordered matrix type by reading and parsing configuration file | ||
| /// @param config_path Path to configuration file or directory containing configuration file | ||
| /// @param error Error struct to indicate success or failure | ||
| void CreateBackwardEuler(const std::string &config_path, Error *error); | ||
|
|
||
| /// @brief Create a Rosenbrock solver of standard-ordered matrix type by reading and parsing configuration file | ||
| /// @param config_path Path to configuration file or directory containing configuration file | ||
| /// @param error Error struct to indicate success or failure | ||
| void CreateRosenbrockStandardOrder(const std::string &config_path, Error *error); | ||
|
|
||
| /// @brief Create a BackwardEuler solver of standard-ordered matrix type by reading and parsing configuration file | ||
| /// @param config_path Path to configuration file or directory containing configuration file | ||
| /// @param error Error struct to indicate success or failure | ||
| void CreateBackwardEulerStandardOrder(const std::string &config_path, Error *error); | ||
|
|
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; | ||
|
|
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?
Added vector order and standard order Backward Euler options to the C API.