Cleanup and adify Reaction and MatReaction #31728

tophmatthews · 2025-10-15T15:55:33Z

#29888 showed a nice way of combining AD and nonAD while including jacobians in the non-ad without muddying up the AD classes. This PR cleans up Reaction in this new method, and combines AD and nonAD MatReaction. Also cleans up tests and clarifies internal naming conventions along the way. Also, deprecating duplicative classes.

Ref #15915

moosebuild · 2025-10-15T22:12:35Z

Job Documentation, step Docs: sync website on 1c6058b wanted to post the following:

View the site here

This comment will be updated on new commits.

moosebuild · 2025-10-16T04:34:06Z

Job Coverage, step Generate coverage on 1c6058b wanted to post the following:

Framework coverage

	17a583	#31728 1c6058
	Total	Total	+/-	New
Rate	85.99%	85.98%	-0.00%	100.00%
Hits	123706	123698	-8	27
Misses	20161	20162	+1	0

Diff coverage report

Full coverage report

Modules coverage

Phase field

	17a583	#31728 1c6058
	Total	Total	+/-	New
Rate	86.10%	86.10%	-	100.00%
Hits	14018	14018	-	2
Misses	2264	2264	-	0

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

moosebuild · 2025-10-16T04:34:07Z

Job Coverage, step Verify coverage on 0dd10cd wanted to post the following:

The following coverage requirement(s) failed:

Failed to generate framework coverage rate (required: 81.0%)

moosebuild · 2025-10-16T14:55:12Z

Job Coverage, step Verify coverage on 0dd10cd wanted to post the following:

The following coverage requirement(s) failed:

Failed to generate framework coverage rate (required: 81.0%)

loganharbour

Check above in #31728 (comment).

New tests are too long!

tophmatthews · 2025-10-16T23:18:46Z

framework/include/kernels/ADMatReaction.h

-  /// Reaction rate material property
-  const ADMaterialProperty<Real> & _reaction_rate;
-};
+#include "MatReaction.h"


Can remove this after MARMOT is patched after this PR is in.

lindsayad · 2025-10-17T18:54:25Z

framework/include/kernels/CoefReaction.h

+class CoefReaction : public CoefReactionTempl<false>
+{
+public:
+  static InputParameters validParams();
+
+  CoefReaction(const InputParameters & parameters);
+
+  using CoefReactionTempl<false>::CoefReactionTempl;
+
+protected:
+  virtual Real computeQpJacobian() override;
+};


what's the reason for these new lines? It seems like things were already working before with the simple typedefs ?

It's due to needing to override computeQpJacobian due to changes in Reaction. I couldn't figure out a better way to do it.

lindsayad

I think you should be able to make the exact same implementation changes without adding additional class hierarchy

framework/src/kernels/CoefReaction.C

framework/include/kernels/CoefReaction.h

lindsayad · 2025-10-18T04:17:11Z

You were probably running into template type dependent base class compilation issues. In the commit I just pushed I show how you can overcome that with using directives

lindsayad · 2025-10-18T04:17:48Z

Another solution would be to qualify all of those data members with this->_i, this->_j etc. but I think this makes for cleaner looking implementations

tophmatthews · 2025-10-20T14:34:03Z

In the commit I just pushed I show how you can overcome that with using directives

interesting...thanks!!

lindsayad · 2025-10-21T17:13:24Z

framework/include/kernels/Reaction.h

+class Reaction : public ReactionTempl<false>
+{
+public:
+  static InputParameters validParams();
+
+  Reaction(const InputParameters & parameters);
+
+  using ReactionTempl<false>::ReactionTempl;
+
+protected:
+  virtual Real computeQpJacobian() override;
+};


oooh I see, computeQpJacobian is defined in ADKernel.h...

lindsayad · 2025-10-21T18:49:48Z

framework/src/kernels/CoefReaction.C

-  return _coef * ReactionTempl<is_ad>::computeQpJacobian();
+  return _coef * _test[_i][_qp] * _rate * _phi[_j][_qp];


why did you change this line? I like what was there before better for code re-use

Huh. I would think you'd want to reduce code duplication?

recoding the phi * test * rate is code duplication

so, which version is desired here?

the old one

lindsayad · 2025-10-21T18:51:02Z

framework/src/kernels/Reaction.C

-  // This function will never be called for the AD version. But because C++ does
-  // not support an optional function declaration based on a template parameter,
-  // we must keep this template for all cases.
-  mooseAssert(!is_ad,
-              "In ADReaction, computeQpJacobian should not be called. Check computeJacobian "
-              "implementation.");


why did you remove this?

It shouldn't get here anyways, but I removed it because I thought the jacobian was never defined for AD version of Reaction, or at least that it was already at final (liked ADKernelValue). All the fancy working before was under this assumption.

I agree it shouldn't get here. The assert is sanity checking just that

yeah I can add that back.

lindsayad · 2025-10-21T18:51:45Z

modules/stochastic_tools/test/src/kernels/MaterialReaction.C


 registerMooseObject("StochasticToolsTestApp", MaterialReaction);

+registerMooseObjectReplaced("StochasticToolsTestApp", MaterialReaction, "01/01/2027 00:00", Matreaction);


Suggested change

registerMooseObjectReplaced("StochasticToolsTestApp", MaterialReaction, "01/01/2027 00:00", Matreaction);

registerMooseObjectReplaced("StochasticToolsTestApp", MaterialReaction, "01/01/2027 00:00", MatReaction);

lindsayad · 2025-10-21T18:52:52Z

test/tests/kernels/mat_reaction/tests

+    input = 'mat_reaction.i'
+    difference_tol = 1e-7
+    design = 'MatReaction.md'
+    requirement = 'The system shall calculate the correct Jacobian of MatReaction.'


Suggested change

requirement = 'The system shall calculate the correct Jacobian of MatReaction.'

requirement = 'The system shall calculate the correct hand-coded Jacobian for a steady-state diffusion-reaction problem.'

lindsayad · 2025-10-21T18:53:05Z

test/tests/kernels/mat_reaction/tests

+    difference_tol = 1e-7
+    design = 'MatReaction.md'
+    requirement = 'The system shall calculate the correct Jacobian of MatReaction.'
+  [../]


Suggested change

[../]

[]

lindsayad · 2025-10-21T18:53:47Z

test/tests/kernels/mat_reaction/tests

+    difference_tol = 1e-7
+    cli_args = 'AD=AD'
+    design = 'ADMatReaction.md'
+    requirement = 'The system shall calculate the correct Jacobian of ADMatReaction using automatic differentiation.'


Suggested change

requirement = 'The system shall calculate the correct Jacobian of ADMatReaction using automatic differentiation.'

requirement = 'The system shall calculate the correct Jacobian via automatic differentiation for a steady-state diffusion-reaction problem.'

lindsayad · 2025-10-21T18:53:55Z

test/tests/kernels/mat_reaction/tests

+    cli_args = 'AD=AD'
+    design = 'ADMatReaction.md'
+    requirement = 'The system shall calculate the correct Jacobian of ADMatReaction using automatic differentiation.'
+  [../]


Suggested change

[../]

[]

moosebuild · 2025-10-21T19:37:47Z

Job Test, step Results summary on 1c6058b wanted to post the following:

Framework test summary

Removed tests

Test	Time (s)
`kernels/ad_mat_reaction.ad_mat_reaction`	2.1
`kernels/ad_reaction.ad_coef_reaction`	1.09
`kernels/ad_mat_reaction.ad_mat_reaction_jac`	1.05
`kernels/ad_reaction.ad_coef_reaction_jac`	0.92
`kernels/ad_reaction.ad_reaction_jac`	0.91
`kernels/ad_reaction.ad_reaction`	0.89

Added tests

Test	Time (s)
`kernels/mat_reaction.ad_mat_reaction`	2.01
`kernels/reaction.exact`	1.19
`kernels/mat_reaction.mat_reaction`	1.11
`kernels/reaction.ad_exact_jac`	1.04
`kernels/mat_reaction.mat_reaction_jac`	1.02
`kernels/mat_reaction.ad_mat_reaction_jac`	1.01
`kernels/reaction.ad_exact`	0.93

Run time changes

Test	Base (s)	Head (s)	+/-
`meshgenerators/mesh_diagnostics_generator.generate/nonconformality`	0.68	1.16	+71.98%
`executioners/fixed_point.tagged_solution_vector/material`	1.12	1.89	+68.04%
`functions/piecewise_linear_from_vectorpostprocessor.test_spatial_data`	0.89	1.43	+61.49%
`functions/piecewise_linear.no_extrap`	0.82	1.31	+59.46%
`userobjects/solution_user_object.write_exodus_second_order`	1.19	1.87	+56.83%
`scalar_kernels/ad_coupled_scalar.exo`	1.04	1.61	+55.61%
`userobjects/execution_order_groups.inter_group_dependence`	0.85	1.31	+53.79%
`fvkernels/fv_burgers.fv_burgers`	0.92	1.41	+52.93%
`userobjects/nearest_point_layered_average.from_file`	1.04	1.58	+51.86%

Modules test summary

Removed tests

None

Added tests

None

Run time changes

Test	Base (s)	Head (s)	+/-
`electromagnetics/test:auxkernels/current_density.errors/ES_electric_field_var`	1.28	2.03	+58.22%
`xfem/test:diffusion_xfem.levelsetcut3d`	1.07	1.65	+54.66%
`richards/test:pressure_pulse.pp02`	1.05	1.59	+52.34%
`stochastic_tools/test:web_server_control.stochastic_control/batch_reset`	3.67	5.54	+50.92%

tophmatthews force-pushed the reaction_adnonad_15915 branch 2 times, most recently from f7109ae to 55e582f Compare October 15, 2025 18:40

tophmatthews added 7 commits October 16, 2025 09:22

cleanup ad/nonad reaction ref idaholab#15915

a53ac86

fixup MatReaction

9afff6c

fixup tests

c97dbd4

fixup coefreaction failures and deprecate duplicative classes

2ed12e4

fix for phasefield action deprecation

c87f2c5

add back ADMatReaction.md to prevent downstream breakage

add17e3

add back header to prevent downstream app breakage

6bd88cd

tophmatthews force-pushed the reaction_adnonad_15915 branch from 0dd10cd to 6bd88cd Compare October 16, 2025 15:22

loganharbour requested changes Oct 16, 2025

View reviewed changes

moosebuild added the PR: Failed but allowed label Oct 16, 2025

speed up tests

660e05e

moosebuild removed the PR: Failed but allowed label Oct 16, 2025

tophmatthews marked this pull request as ready for review October 16, 2025 23:17

tophmatthews requested review from dschwen, grmnptr, lindsayad and zachmprince as code owners October 16, 2025 23:17

tophmatthews requested a review from loganharbour October 16, 2025 23:17

tophmatthews commented Oct 16, 2025

View reviewed changes

lindsayad reviewed Oct 17, 2025

View reviewed changes

lindsayad reviewed Oct 18, 2025

View reviewed changes

framework/src/kernels/CoefReaction.C Outdated Show resolved Hide resolved

framework/src/kernels/CoefReaction.C Outdated Show resolved Hide resolved

framework/include/kernels/CoefReaction.h Outdated Show resolved Hide resolved

Demonstrate overcoming template type dependent bases

0e20ff1

GiudGiud assigned lindsayad Oct 20, 2025

lindsayad requested changes Oct 21, 2025

View reviewed changes

tophmatthews added 2 commits October 21, 2025 12:23

remove extra Reaction definition

ac55f54

make virutal

1c6058b

lindsayad requested changes Oct 21, 2025

View reviewed changes

		return _coef * ReactionTempl<is_ad>::computeQpJacobian();
		return _coef * _test[_i][_qp] * _rate * _phi[_j][_qp];


		registerMooseObject("StochasticToolsTestApp", MaterialReaction);

		registerMooseObjectReplaced("StochasticToolsTestApp", MaterialReaction, "01/01/2027 00:00", Matreaction);

	requirement = 'The system shall calculate the correct Jacobian of MatReaction.'
	requirement = 'The system shall calculate the correct hand-coded Jacobian for a steady-state diffusion-reaction problem.'

Cleanup and adify Reaction and MatReaction #31728

Are you sure you want to change the base?

Cleanup and adify Reaction and MatReaction #31728

Uh oh!

Conversation

tophmatthews commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moosebuild commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moosebuild commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Framework coverage

Modules coverage

Phase field

Full coverage reports

Uh oh!

moosebuild commented Oct 16, 2025

Uh oh!

moosebuild commented Oct 16, 2025

Uh oh!

loganharbour left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tophmatthews Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lindsayad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lindsayad commented Oct 18, 2025

Uh oh!

lindsayad commented Oct 18, 2025

Uh oh!

tophmatthews commented Oct 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moosebuild commented Oct 21, 2025

Framework test summary

tophmatthews commented Oct 15, 2025 •

edited

Loading

moosebuild commented Oct 15, 2025 •

edited

Loading

moosebuild commented Oct 16, 2025 •

edited

Loading

loganharbour left a comment •

edited

Loading

tophmatthews Oct 17, 2025 •

edited

Loading