Combine ad nonad conservative advection 15915

[tophmatthews]

Combines AD/nonAD implementations of ConservativeAdvection. There was a bit of a challenge due velocity being pulled in as a variable in ConservativeAdvection, and velocity pulled in as a material property in ADConservativeAdvection. I still have to do ADMassLumpedTimeDerivative separately and ADInfluxBC separately to enable a true AD upwinding capability.

Ref #15915

[tophmatthews]

I need some help on this: I am renaming a param and a coupledvar, and choosing which one to use in the constructor based on isParamValid. isParamValid will pick up a number in the input file applied to a coupledVar, but when I call coupledGenericVectorValue, it does not pick up on this:

*** ERROR ***
Attempted to retrieve default value for coupled variable 'velocity_variable' when none was provided. 

There are three reasons why this may have occurred:
 1. The other version of params.addCoupledVar() should be used in order to provide a default value. 
 2. This should have been a required coupled variable added with params.addRequiredCoupledVar() 
 3. The call to get the coupled value should have been properly guarded with isCoupled()

Help!

[moosebuild]

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

View the site here

This comment will be updated on new commits.

[tophmatthews]

I'm guessing it's something missing in Coupleable::getDefaultVectorValue that Coupleable::getDefaultValue doesn't suffer from...

[tophmatthews]

no that's not it, it's something in renameParam that doesn't transfer the default correctly!

[tophmatthews]

UGH! No, it all comes down to isParamValid being true due to a non-variable value given in the input file, but coupledGenericVectorValue requiring a coupled variable! It doesn't have to be coupled!

[tophmatthews]

Here's what I've learned when using params.deprecateCoupledVar("old", "new", "12/31/2025");:

• if either old or new are provided, then both isParamValid and isParamSetByUser is true for both old and new
• if neither old nor new are provided, then both isParamValid and isParamSetByUser is false for both old and new
• if old is provided, coupledGenericVectorValue<is_ad>("old") works, coupledGenericVectorValue<is_ad>("new") gets stuck on Attempted to retrieve default value for coupled variable 'new' when none was provided.
• if new is provided, coupledGenericVectorValue<is_ad>("new") works, coupledGenericVectorValue<is_ad>("old") gets stuck on Attempted to retrieve default value for coupled variable 'old' when none was provided.

[tophmatthews]

For the MaterialPropertyName deprecation in the ad version, I was able to use addDeprecatedParam. For the coupled variable in the non-ad version, I was unable to use deprecateCoupledVar to pass the correct input parameter to use. I added a space to the name to prevent it from doing all the behind the scenes default passing. Not ideal, but should work.

[tophmatthews]

I put in what should make sense...but it just doesn't work...

[moosebuild]

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

Framework coverage

	49bfce	#29889 e875c9
	Total	Total	+/-	New
Rate	85.44%	85.44%	+0.00%	85.87%
Hits	112378	112399	+21	79
Misses	19152	19152	-	13

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

• framework
• chemical_reactions
• combined
• contact
• electromagnetics
• external_petsc_solver
• fluid_properties
• fsi
• functional_expansion_tools
• geochemistry
• heat_transfer
• level_set
• misc
• navier_stokes
• optimization
• peridynamics
• phase_field
• porous_flow
• ray_tracing
• rdg
• reactor
• richards
• scalar_transport
• solid_mechanics
• solid_properties
• stochastic_tools
• subchannel
• thermal_hydraulics
• xfem

Warnings

• framework new line coverage rate 85.87% is less than the suggested 90.0%

This comment will be updated on new commits.

[tophmatthews]

Well I'm a bit bummed because I spent a ton of time combining these, but I'm not totally convinced that upwinding can be applied to AD versions since it overwrites the jacobian definition...probably need your insight @lindsayad

[GiudGiud]

if @lindsayad does not get to it I can take this over post MOOSE-conference

[lindsayad]

what is the issue? Is there an upwind test that's not working with the AD version?

[tophmatthews]

what is the issue? Is there an upwind test that's not working with the AD version?

No, there isn't a particular issue. I need to get the time kernels into AD before I can really test upwinding. It was more of a concern that I was monkeying around with computeJacobian to enable upwinding, and didn't want to mess with the AD and tagging capability built in. A careful review would put my concerns at ease, and then I'll go update the time kernels to AD.

[lindsayad]

I'm probably going to push some changes to this

[lindsayad]

Need to wait on #29817 to get DenseVector::operator[]

[tophmatthews]

Cool thanks @lindsayad ! See I knew I was missing something...

[lindsayad]

This will further need to wait on libMesh/libmesh#4098 but we are planning to do another libmesh update very soon

[lindsayad]

Major bummer that the most recent libmesh update merged today did not bring in the begin/end PR 😭

[lindsayad]

@tophmatthews looks like you just need a documentation fixup here

[lindsayad]

@GiudGiud could you review my commits please?

[GiudGiud]

Python failures tied to a package change and unrelated to this PR