Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove solver_.parameters_ #668

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Remove solver_.parameters_ #668

wants to merge 48 commits into from

Conversation

montythind
Copy link
Contributor

@montythind montythind commented Sep 24, 2024

This PR includes moving Parameters
absolute tolerance and relative tolearnce to state from solver

Note:
I have left
std::vector absolute_tolerance_;
double relative_tolerance_{ 1e-6 };

in rosenbrock_solver_prarameter.hpp as the CUDA tests are failing because tolerance doesnt get copied over to the device. Issue occurs in CudaRosenbrockSolver constructor when doing this->parameters_.absolute_tolerance_.size(); it comes out as zero

We can potential get rid of this line in solver_builder.inl. This is also a temporary. solution.
options.absolute_tolerance_ = state_parameters.absolute_tolerance_;

Along with that i have commented out unit tests till we don't have a solution how to deal with tolerance.

closes #621

@montythind
Copy link
Contributor Author

I do see some calls where solver.solver_.parameters_ is not being modifiedbut rather the value is getting used. Example:

EXPECT_EQ(solver.solver_.parameters_.absolute_tolerance_[state.variable_map_["Ar"]], 1.0e-12);
std::vector atol = gpu_solver.solver_.parameters_.absolute_tolerance_;

I am unsure if we need to change these as well?

@K20shores @mattldawson

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.28%. Comparing base (25dee9b) to head (69fbc05).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #668      +/-   ##
==========================================
+ Coverage   93.97%   94.28%   +0.31%     
==========================================
  Files          61       61              
  Lines        4163     4179      +16     
==========================================
+ Hits         3912     3940      +28     
+ Misses        251      239      -12     

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

Copy link
Collaborator

@K20shores K20shores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right direction, but we still have some more work to do. Any mechanism-specific thing (like tolerances) need to be moved somewhere else. We should spend some time figuring out where they need to go. Also, the parameters need to be any set of paramters (rosenbrock, backward euler, etc.)

include/micm/solver/solver.hpp Outdated Show resolved Hide resolved
@montythind montythind force-pushed the allowSolversToModify_621 branch from d059401 to 32a16e2 Compare October 1, 2024 21:07
@montythind montythind changed the title Draft PR: Remove solver_.parameters_ Remove solver_.parameters_ Oct 16, 2024
@montythind montythind requested a review from K20shores October 16, 2024 20:28
Copy link
Collaborator

@sjsprecious sjsprecious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @montythind for working on this PR. I observed the following failures on Derecho:

         15 - cuda_rosenbrock (SEGFAULT)
         31 - rosenbrock (SEGFAULT)
         45 - regression_test_solve (NUMERICAL)
         46 - chapman_integration (SEGFAULT)
         47 - analytical_rosenbrock_integration (NUMERICAL)
         48 - analytical_backward_euler (NUMERICAL)
         49 - terminator (NUMERICAL)
         50 - integrated_reaction_rates (NUMERICAL)
         52 - README_example (NUMERICAL)
         53 - solve_results (NUMERICAL)
         54 - multiple_grid_cells (NUMERICAL)
         55 - rate_constants_no_user_defined_example_by_hand (NUMERICAL)
         56 - rate_constants_user_defined_example_by_hand (NUMERICAL)
         57 - solver_configuration (NUMERICAL)
         59 - rate_constants_no_user_defined_example_with_config (NUMERICAL)
         60 - rate_constants_user_defined_example_with_config (NUMERICAL)
         61 - carbon_bond_5_example (NUMERICAL)
         62 - chapman_example (NUMERICAL)
         63 - robertson_example (NUMERICAL)
         64 - ts1_example (NUMERICAL)

include/micm/solver/rosenbrock_solver_parameters.hpp Outdated Show resolved Hide resolved
@montythind
Copy link
Contributor Author

I am not sure what happened but after my latest pull and merge- lot of tests failed. I am looking what caused the test failures

@sjsprecious
Copy link
Collaborator

Interesting. I just ran the CUDA tests on my branch for the LU decomposition rename PR and it ran much faster.

Test project /glade/derecho/scratch/sunjian/micm/build
      Start  1: solver_config
 1/64 Test  #1: solver_config ........................................   Passed    0.68 sec
      Start  2: arrhenius_config
 2/64 Test  #2: arrhenius_config .....................................   Passed    0.70 sec
      Start  3: branched_config
 3/64 Test  #3: branched_config ......................................   Passed    0.80 sec
      Start  4: emission_config
 4/64 Test  #4: emission_config ......................................   Passed    0.28 sec
      Start  5: first_order_loss_config
 5/64 Test  #5: first_order_loss_config ..............................   Passed    0.40 sec
      Start  6: photolysis_config
 6/64 Test  #6: photolysis_config ....................................   Passed    0.67 sec
      Start  7: surface_config
 7/64 Test  #7: surface_config .......................................   Passed    0.42 sec
      Start  8: ternary_chemical_activation_config
 8/64 Test  #8: ternary_chemical_activation_config ...................   Passed    0.65 sec
      Start  9: troe_config
 9/64 Test  #9: troe_config ..........................................   Passed    0.33 sec
      Start 10: tunneling_config
10/64 Test #10: tunneling_config .....................................   Passed    0.23 sec
      Start 11: user_defined_config
11/64 Test #11: user_defined_config ..................................   Passed    0.39 sec
      Start 12: cuda_process_set
12/64 Test #12: cuda_process_set .....................................   Passed    6.24 sec
      Start 13: cuda_doolittle_lu_decomposition
13/64 Test #13: cuda_doolittle_lu_decomposition ......................   Passed    6.24 sec
      Start 14: cuda_linear_solver
14/64 Test #14: cuda_linear_solver ...................................   Passed    3.10 sec
      Start 15: cuda_rosenbrock
15/64 Test #15: cuda_rosenbrock ......................................   Passed    2.37 sec
      Start 16: cuda_solver_builder
16/64 Test #16: cuda_solver_builder ..................................   Passed    0.47 sec
      Start 17: cuda_dense_matrix
17/64 Test #17: cuda_dense_matrix ....................................   Passed    0.52 sec
      Start 18: cuda_sparse_matrix
18/64 Test #18: cuda_sparse_matrix ...................................   Passed    0.47 sec
      Start 19: process
19/64 Test #19: process ..............................................   Passed    0.05 sec
      Start 20: process_set
20/64 Test #20: process_set ..........................................   Passed    0.07 sec
      Start 21: arrhenius_rate_constant
21/64 Test #21: arrhenius_rate_constant ..............................   Passed    0.06 sec
      Start 22: branched_rate_constant
22/64 Test #22: branched_rate_constant ...............................   Passed    0.05 sec
      Start 23: surface_rate_constant
23/64 Test #23: surface_rate_constant ................................   Passed    0.06 sec
      Start 24: ternary_chemical_activation_rate_constant
24/64 Test #24: ternary_chemical_activation_rate_constant ............   Passed    0.05 sec
      Start 25: troe_rate_constant
25/64 Test #25: troe_rate_constant ...................................   Passed    0.03 sec
      Start 26: tunneling_rate_constant
26/64 Test #26: tunneling_rate_constant ..............................   Passed    0.10 sec
      Start 27: user_defined_rate_constant
27/64 Test #27: user_defined_rate_constant ...........................   Passed    0.03 sec
      Start 28: chapman_ode_solver
28/64 Test #28: chapman_ode_solver ...................................   Passed    0.10 sec
      Start 29: linear_solver
29/64 Test #29: linear_solver ........................................   Passed    0.13 sec
      Start 30: doolittle_lu_decomposition
30/64 Test #30: doolittle_lu_decomposition ...........................   Passed    0.10 sec
      Start 31: rosenbrock
31/64 Test #31: rosenbrock ...........................................   Passed    0.04 sec
      Start 32: state
32/64 Test #32: state ................................................   Passed    0.07 sec
      Start 33: backward_euler
33/64 Test #33: backward_euler .......................................   Passed    0.04 sec
      Start 34: solver_builder
34/64 Test #34: solver_builder .......................................   Passed    0.05 sec
      Start 35: phase
35/64 Test #35: phase ................................................   Passed    0.05 sec
      Start 36: species
36/64 Test #36: species ..............................................   Passed    0.10 sec
      Start 37: system
37/64 Test #37: system ...............................................   Passed    0.08 sec
      Start 38: matrix
38/64 Test #38: matrix ...............................................   Passed    0.07 sec
      Start 39: sparse_matrix_standard_ordering
39/64 Test #39: sparse_matrix_standard_ordering ......................   Passed    0.04 sec
      Start 40: sparse_matrix_vector_ordering
40/64 Test #40: sparse_matrix_vector_ordering ........................   Passed    0.05 sec
      Start 41: vector_matrix
41/64 Test #41: vector_matrix ........................................   Passed    0.04 sec
      Start 42: version
42/64 Test #42: version ..............................................   Passed    0.06 sec
      Start 43: regression_test_rosenbrock_p_force
43/64 Test #43: regression_test_rosenbrock_p_force ...................   Passed    0.03 sec
      Start 44: regression_test_rosenbrock_dforce_dy
44/64 Test #44: regression_test_rosenbrock_dforce_dy .................   Passed    0.05 sec
      Start 45: regression_test_solve
45/64 Test #45: regression_test_solve ................................   Passed    0.04 sec
      Start 46: chapman_integration
46/64 Test #46: chapman_integration ..................................   Passed    0.05 sec
      Start 47: analytical_rosenbrock_integration
47/64 Test #47: analytical_rosenbrock_integration ....................   Passed    3.14 sec
      Start 48: analytical_backward_euler
48/64 Test #48: analytical_backward_euler ............................   Passed    0.83 sec
      Start 49: terminator
49/64 Test #49: terminator ...........................................   Passed    0.18 sec
      Start 50: integrated_reaction_rates
50/64 Test #50: integrated_reaction_rates ............................   Passed    0.27 sec
      Start 51: cuda_analytical_rosenbrock_integration
51/64 Test #51: cuda_analytical_rosenbrock_integration ...............   Passed  204.09 sec
      Start 52: README_example
52/64 Test #52: README_example .......................................   Passed    0.25 sec
      Start 53: solve_results
53/64 Test #53: solve_results ........................................   Passed    0.04 sec
      Start 54: multiple_grid_cells
54/64 Test #54: multiple_grid_cells ..................................   Passed    0.08 sec
      Start 55: rate_constants_no_user_defined_example_by_hand
55/64 Test #55: rate_constants_no_user_defined_example_by_hand .......   Passed    0.03 sec
      Start 56: rate_constants_user_defined_example_by_hand
56/64 Test #56: rate_constants_user_defined_example_by_hand ..........   Passed    0.04 sec
      Start 57: solver_configuration
57/64 Test #57: solver_configuration .................................   Passed    0.25 sec
      Start 58: vectorized_matrix_solver
58/64 Test #58: vectorized_matrix_solver .............................   Passed    0.07 sec
      Start 59: rate_constants_no_user_defined_example_with_config
59/64 Test #59: rate_constants_no_user_defined_example_with_config ...   Passed    0.10 sec
      Start 60: rate_constants_user_defined_example_with_config
60/64 Test #60: rate_constants_user_defined_example_with_config ......   Passed    0.13 sec
      Start 61: carbon_bond_5_example
61/64 Test #61: carbon_bond_5_example ................................   Passed    0.16 sec
      Start 62: chapman_example
62/64 Test #62: chapman_example ......................................   Passed    0.06 sec
      Start 63: robertson_example
63/64 Test #63: robertson_example ....................................   Passed    0.08 sec
      Start 64: ts1_example
64/64 Test #64: ts1_example ..........................................   Passed    0.65 sec

Can you run it on a scratch directory in case it is an issue due to the filesystem for the home directory?

@K20shores
Copy link
Collaborator

Interesting. I just ran the CUDA tests on my branch for the LU decomposition rename PR and it ran much faster.

Test project /glade/derecho/scratch/sunjian/micm/build
      Start  1: solver_config
 1/64 Test  #1: solver_config ........................................   Passed    0.68 sec
      Start  2: arrhenius_config
 2/64 Test  #2: arrhenius_config .....................................   Passed    0.70 sec
      Start  3: branched_config
 3/64 Test  #3: branched_config ......................................   Passed    0.80 sec
      Start  4: emission_config
 4/64 Test  #4: emission_config ......................................   Passed    0.28 sec
      Start  5: first_order_loss_config
 5/64 Test  #5: first_order_loss_config ..............................   Passed    0.40 sec
      Start  6: photolysis_config
 6/64 Test  #6: photolysis_config ....................................   Passed    0.67 sec
      Start  7: surface_config
 7/64 Test  #7: surface_config .......................................   Passed    0.42 sec
      Start  8: ternary_chemical_activation_config
 8/64 Test  #8: ternary_chemical_activation_config ...................   Passed    0.65 sec
      Start  9: troe_config
 9/64 Test  #9: troe_config ..........................................   Passed    0.33 sec
      Start 10: tunneling_config
10/64 Test #10: tunneling_config .....................................   Passed    0.23 sec
      Start 11: user_defined_config
11/64 Test #11: user_defined_config ..................................   Passed    0.39 sec
      Start 12: cuda_process_set
12/64 Test #12: cuda_process_set .....................................   Passed    6.24 sec
      Start 13: cuda_doolittle_lu_decomposition
13/64 Test #13: cuda_doolittle_lu_decomposition ......................   Passed    6.24 sec
      Start 14: cuda_linear_solver
14/64 Test #14: cuda_linear_solver ...................................   Passed    3.10 sec
      Start 15: cuda_rosenbrock
15/64 Test #15: cuda_rosenbrock ......................................   Passed    2.37 sec
      Start 16: cuda_solver_builder
16/64 Test #16: cuda_solver_builder ..................................   Passed    0.47 sec
      Start 17: cuda_dense_matrix
17/64 Test #17: cuda_dense_matrix ....................................   Passed    0.52 sec
      Start 18: cuda_sparse_matrix
18/64 Test #18: cuda_sparse_matrix ...................................   Passed    0.47 sec
      Start 19: process
19/64 Test #19: process ..............................................   Passed    0.05 sec
      Start 20: process_set
20/64 Test #20: process_set ..........................................   Passed    0.07 sec
      Start 21: arrhenius_rate_constant
21/64 Test #21: arrhenius_rate_constant ..............................   Passed    0.06 sec
      Start 22: branched_rate_constant
22/64 Test #22: branched_rate_constant ...............................   Passed    0.05 sec
      Start 23: surface_rate_constant
23/64 Test #23: surface_rate_constant ................................   Passed    0.06 sec
      Start 24: ternary_chemical_activation_rate_constant
24/64 Test #24: ternary_chemical_activation_rate_constant ............   Passed    0.05 sec
      Start 25: troe_rate_constant
25/64 Test #25: troe_rate_constant ...................................   Passed    0.03 sec
      Start 26: tunneling_rate_constant
26/64 Test #26: tunneling_rate_constant ..............................   Passed    0.10 sec
      Start 27: user_defined_rate_constant
27/64 Test #27: user_defined_rate_constant ...........................   Passed    0.03 sec
      Start 28: chapman_ode_solver
28/64 Test #28: chapman_ode_solver ...................................   Passed    0.10 sec
      Start 29: linear_solver
29/64 Test #29: linear_solver ........................................   Passed    0.13 sec
      Start 30: doolittle_lu_decomposition
30/64 Test #30: doolittle_lu_decomposition ...........................   Passed    0.10 sec
      Start 31: rosenbrock
31/64 Test #31: rosenbrock ...........................................   Passed    0.04 sec
      Start 32: state
32/64 Test #32: state ................................................   Passed    0.07 sec
      Start 33: backward_euler
33/64 Test #33: backward_euler .......................................   Passed    0.04 sec
      Start 34: solver_builder
34/64 Test #34: solver_builder .......................................   Passed    0.05 sec
      Start 35: phase
35/64 Test #35: phase ................................................   Passed    0.05 sec
      Start 36: species
36/64 Test #36: species ..............................................   Passed    0.10 sec
      Start 37: system
37/64 Test #37: system ...............................................   Passed    0.08 sec
      Start 38: matrix
38/64 Test #38: matrix ...............................................   Passed    0.07 sec
      Start 39: sparse_matrix_standard_ordering
39/64 Test #39: sparse_matrix_standard_ordering ......................   Passed    0.04 sec
      Start 40: sparse_matrix_vector_ordering
40/64 Test #40: sparse_matrix_vector_ordering ........................   Passed    0.05 sec
      Start 41: vector_matrix
41/64 Test #41: vector_matrix ........................................   Passed    0.04 sec
      Start 42: version
42/64 Test #42: version ..............................................   Passed    0.06 sec
      Start 43: regression_test_rosenbrock_p_force
43/64 Test #43: regression_test_rosenbrock_p_force ...................   Passed    0.03 sec
      Start 44: regression_test_rosenbrock_dforce_dy
44/64 Test #44: regression_test_rosenbrock_dforce_dy .................   Passed    0.05 sec
      Start 45: regression_test_solve
45/64 Test #45: regression_test_solve ................................   Passed    0.04 sec
      Start 46: chapman_integration
46/64 Test #46: chapman_integration ..................................   Passed    0.05 sec
      Start 47: analytical_rosenbrock_integration
47/64 Test #47: analytical_rosenbrock_integration ....................   Passed    3.14 sec
      Start 48: analytical_backward_euler
48/64 Test #48: analytical_backward_euler ............................   Passed    0.83 sec
      Start 49: terminator
49/64 Test #49: terminator ...........................................   Passed    0.18 sec
      Start 50: integrated_reaction_rates
50/64 Test #50: integrated_reaction_rates ............................   Passed    0.27 sec
      Start 51: cuda_analytical_rosenbrock_integration
51/64 Test #51: cuda_analytical_rosenbrock_integration ...............   Passed  204.09 sec
      Start 52: README_example
52/64 Test #52: README_example .......................................   Passed    0.25 sec
      Start 53: solve_results
53/64 Test #53: solve_results ........................................   Passed    0.04 sec
      Start 54: multiple_grid_cells
54/64 Test #54: multiple_grid_cells ..................................   Passed    0.08 sec
      Start 55: rate_constants_no_user_defined_example_by_hand
55/64 Test #55: rate_constants_no_user_defined_example_by_hand .......   Passed    0.03 sec
      Start 56: rate_constants_user_defined_example_by_hand
56/64 Test #56: rate_constants_user_defined_example_by_hand ..........   Passed    0.04 sec
      Start 57: solver_configuration
57/64 Test #57: solver_configuration .................................   Passed    0.25 sec
      Start 58: vectorized_matrix_solver
58/64 Test #58: vectorized_matrix_solver .............................   Passed    0.07 sec
      Start 59: rate_constants_no_user_defined_example_with_config
59/64 Test #59: rate_constants_no_user_defined_example_with_config ...   Passed    0.10 sec
      Start 60: rate_constants_user_defined_example_with_config
60/64 Test #60: rate_constants_user_defined_example_with_config ......   Passed    0.13 sec
      Start 61: carbon_bond_5_example
61/64 Test #61: carbon_bond_5_example ................................   Passed    0.16 sec
      Start 62: chapman_example
62/64 Test #62: chapman_example ......................................   Passed    0.06 sec
      Start 63: robertson_example
63/64 Test #63: robertson_example ....................................   Passed    0.08 sec
      Start 64: ts1_example
64/64 Test #64: ts1_example ..........................................   Passed    0.65 sec

Can you run it on a scratch directory in case it is an issue due to the filesystem for the home directory?

Oh man. That's drastically longer. I will try scratch and see what happens

@K20shores
Copy link
Collaborator

@sjsprecious I guess it was a scratch issue?

Running tests...
Test project /glade/derecho/scratch/kshores/micm/build
      Start  1: solver_config
 1/64 Test  #1: solver_config ........................................   Passed    0.19 sec
      Start  2: arrhenius_config
 2/64 Test  #2: arrhenius_config .....................................   Passed    0.13 sec
      Start  3: branched_config
 3/64 Test  #3: branched_config ......................................   Passed    0.11 sec
      Start  4: emission_config
 4/64 Test  #4: emission_config ......................................   Passed    0.09 sec
      Start  5: first_order_loss_config
 5/64 Test  #5: first_order_loss_config ..............................   Passed    0.05 sec
      Start  6: photolysis_config
 6/64 Test  #6: photolysis_config ....................................   Passed    0.10 sec
      Start  7: surface_config
 7/64 Test  #7: surface_config .......................................   Passed    0.10 sec
      Start  8: ternary_chemical_activation_config
 8/64 Test  #8: ternary_chemical_activation_config ...................   Passed    0.06 sec
      Start  9: troe_config
 9/64 Test  #9: troe_config ..........................................   Passed    0.05 sec
      Start 10: tunneling_config
10/64 Test #10: tunneling_config .....................................   Passed    0.07 sec
      Start 11: user_defined_config
11/64 Test #11: user_defined_config ..................................   Passed    0.10 sec
      Start 12: cuda_process_set
12/64 Test #12: cuda_process_set .....................................   Passed    5.88 sec
      Start 13: cuda_lu_decomposition
13/64 Test #13: cuda_lu_decomposition ................................   Passed    6.23 sec
      Start 14: cuda_linear_solver
14/64 Test #14: cuda_linear_solver ...................................   Passed    3.23 sec
      Start 15: cuda_rosenbrock
15/64 Test #15: cuda_rosenbrock ......................................   Passed    2.45 sec
      Start 16: cuda_solver_builder
16/64 Test #16: cuda_solver_builder ..................................   Passed    0.45 sec
      Start 17: cuda_dense_matrix
17/64 Test #17: cuda_dense_matrix ....................................   Passed    0.46 sec
      Start 18: cuda_sparse_matrix
18/64 Test #18: cuda_sparse_matrix ...................................   Passed    0.53 sec
      Start 19: process
19/64 Test #19: process ..............................................   Passed    0.05 sec
      Start 20: process_set
20/64 Test #20: process_set ..........................................   Passed    0.08 sec
      Start 21: arrhenius_rate_constant
21/64 Test #21: arrhenius_rate_constant ..............................   Passed    0.05 sec
      Start 22: branched_rate_constant
22/64 Test #22: branched_rate_constant ...............................   Passed    0.06 sec
      Start 23: surface_rate_constant
23/64 Test #23: surface_rate_constant ................................   Passed    0.04 sec
      Start 24: ternary_chemical_activation_rate_constant
24/64 Test #24: ternary_chemical_activation_rate_constant ............   Passed    0.05 sec
      Start 25: troe_rate_constant
25/64 Test #25: troe_rate_constant ...................................   Passed    0.06 sec
      Start 26: tunneling_rate_constant
26/64 Test #26: tunneling_rate_constant ..............................   Passed    0.04 sec
      Start 27: user_defined_rate_constant
27/64 Test #27: user_defined_rate_constant ...........................   Passed    0.05 sec
      Start 28: chapman_ode_solver
28/64 Test #28: chapman_ode_solver ...................................   Passed    0.09 sec
      Start 29: linear_solver
29/64 Test #29: linear_solver ........................................   Passed    0.14 sec
      Start 30: lu_decomposition
30/64 Test #30: lu_decomposition .....................................   Passed    0.06 sec
      Start 31: rosenbrock
31/64 Test #31: rosenbrock ...........................................   Passed    0.05 sec
      Start 32: state
32/64 Test #32: state ................................................   Passed    0.05 sec
      Start 33: backward_euler
33/64 Test #33: backward_euler .......................................   Passed    0.06 sec
      Start 34: solver_builder
34/64 Test #34: solver_builder .......................................   Passed    0.04 sec
      Start 35: phase
35/64 Test #35: phase ................................................   Passed    0.05 sec
      Start 36: species
36/64 Test #36: species ..............................................   Passed    0.07 sec
      Start 37: system
37/64 Test #37: system ...............................................   Passed    0.04 sec
      Start 38: matrix
38/64 Test #38: matrix ...............................................   Passed    0.07 sec
      Start 39: sparse_matrix_standard_ordering
39/64 Test #39: sparse_matrix_standard_ordering ......................   Passed    0.10 sec
      Start 40: sparse_matrix_vector_ordering
40/64 Test #40: sparse_matrix_vector_ordering ........................   Passed    0.05 sec
      Start 41: vector_matrix
41/64 Test #41: vector_matrix ........................................   Passed    0.05 sec
      Start 42: version
42/64 Test #42: version ..............................................   Passed    0.06 sec
      Start 43: regression_test_rosenbrock_p_force
43/64 Test #43: regression_test_rosenbrock_p_force ...................   Passed    0.05 sec
      Start 44: regression_test_rosenbrock_dforce_dy
44/64 Test #44: regression_test_rosenbrock_dforce_dy .................   Passed    0.07 sec
      Start 45: regression_test_solve
45/64 Test #45: regression_test_solve ................................   Passed    0.05 sec
      Start 46: chapman_integration
46/64 Test #46: chapman_integration ..................................   Passed    0.06 sec
      Start 47: analytical_rosenbrock_integration
47/64 Test #47: analytical_rosenbrock_integration ....................   Passed    2.94 sec
      Start 48: analytical_backward_euler
48/64 Test #48: analytical_backward_euler ............................   Passed    0.77 sec
      Start 49: terminator
49/64 Test #49: terminator ...........................................   Passed    0.22 sec
      Start 50: integrated_reaction_rates
50/64 Test #50: integrated_reaction_rates ............................   Passed    0.04 sec
      Start 51: cuda_analytical_rosenbrock_integration
51/64 Test #51: cuda_analytical_rosenbrock_integration ...............   Passed  241.47 sec
      Start 52: README_example
52/64 Test #52: README_example .......................................   Passed    0.06 sec
      Start 53: solve_results
53/64 Test #53: solve_results ........................................   Passed    0.04 sec
      Start 54: multiple_grid_cells
54/64 Test #54: multiple_grid_cells ..................................   Passed    0.07 sec
      Start 55: rate_constants_no_user_defined_example_by_hand
55/64 Test #55: rate_constants_no_user_defined_example_by_hand .......   Passed    0.04 sec
      Start 56: rate_constants_user_defined_example_by_hand
56/64 Test #56: rate_constants_user_defined_example_by_hand ..........   Passed    0.06 sec
      Start 57: solver_configuration
57/64 Test #57: solver_configuration .................................   Passed    0.10 sec
      Start 58: vectorized_matrix_solver
58/64 Test #58: vectorized_matrix_solver .............................   Passed    0.05 sec
      Start 59: rate_constants_no_user_defined_example_with_config
59/64 Test #59: rate_constants_no_user_defined_example_with_config ...   Passed    0.05 sec
      Start 60: rate_constants_user_defined_example_with_config
60/64 Test #60: rate_constants_user_defined_example_with_config ......   Passed    0.06 sec
      Start 61: carbon_bond_5_example
61/64 Test #61: carbon_bond_5_example ................................   Passed    0.14 sec
      Start 62: chapman_example
62/64 Test #62: chapman_example ......................................   Passed    0.01 sec
      Start 63: robertson_example
63/64 Test #63: robertson_example ....................................   Passed    0.01 sec
      Start 64: ts1_example
64/64 Test #64: ts1_example ..........................................   Passed    0.61 sec

@sjsprecious
Copy link
Collaborator

@K20shores The new result is consistent with mine now. I noticed that you were running the CUDA tests on your home directory on Derecho and I think that was the problem.

@K20shores K20shores requested a review from sjsprecious January 9, 2025 20:51
Copy link
Collaborator

@sjsprecious sjsprecious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @K20shores and @montythind for working on this and addressing all my comments. It looks great to me and I could confirm that all the CPU & GPU tests pass on Derecho.

Copy link
Collaborator

@mattldawson mattldawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one minor question, but nothing that should hold up merging

Comment on lines +71 to +72
solver_.parameters_ = params;
return solver_.Solve(time_step, state);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to have parameters_ be a data member of solver_ still? Could we just pass the parameters to the solve function as an argument by reference and avoid the copy?

Copy link
Collaborator

@boulderdaze boulderdaze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I just have a couple questions and suggestions.

Comment on lines 118 to 119
{
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest initializing the struct variable in the other two constructors that don’t pass relative tolerance as a function argument.

SolverParameters(System&& system, std::vector<Process>&& processes, RosenbrockSolverParameters&& parameters)
    : system_(system),
      processes_(processes),
      parameters_(parameters)
{
  relative_tolerance_ = 0.0;
}

@@ -959,7 +968,7 @@ namespace micm
SolverParameters GetSolverParams()
{
return SolverParameters(
std::move(System(this->gas_phase_, this->phases_)), std::move(this->processes_), std::move(this->parameters_));
std::move(System(this->gas_phase_, this->phases_)), std::move(this->processes_), std::move(this->parameters_), std::move(this->relative_tolerance_));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove std::move? It doesn’t match the constructor and I think there's no real need for that for a double.

this->relative_tolerance_;

@@ -65,6 +65,13 @@ namespace micm
return result;
}

// Overloaded Solve function to change parameters
SolverResult Solve(double time_step, StatePolicy& state, SolverParametersType& params)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can they be passed as const ref since the objects are not modified?

SolverResult Solve(double time_step, const StatePolicy& state, const SolverParametersType& params)

Comment on lines +425 to +427


this->SetAbsoluteTolerances(state_parameters.absolute_tolerance_, species_map);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this additional new line isn't necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants