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

Finish support for AD time derivatives of dofs #28857

Open
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

GiudGiud
Copy link
Contributor

@GiudGiud GiudGiud commented Oct 15, 2024

also includes a bug fix for second order AD dot dots
closes #28859
refs #28867

@GiudGiud GiudGiud self-assigned this Oct 15, 2024
@GiudGiud GiudGiud force-pushed the PR_lumped branch 2 times, most recently from 1bda2b7 to f5a8d37 Compare October 16, 2024 00:38
@GiudGiud GiudGiud marked this pull request as ready for review October 16, 2024 00:38
@GiudGiud GiudGiud changed the title Add support for time derivatives of dofs Finish support for time derivatives of dofs Oct 16, 2024
@moosebuild
Copy link
Contributor

moosebuild commented Oct 16, 2024

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

View the site here

This comment will be updated on new commits.

@GiudGiud GiudGiud changed the title Finish support for time derivatives of dofs Finish support for AD time derivatives of dofs Oct 16, 2024
… nonAD version)

Add support for nodal AD time derivatives, with errors for unsupported cases
closes idaholab#28859 refs idaholab#28867
@moosebuild
Copy link
Contributor

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

Framework coverage

b3924e #28857 e71c34
Total Total +/- New
Rate 85.05% 85.05% +0.00% 68.18%
Hits 106298 106333 +35 30
Misses 18691 18696 +5 14

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

Warnings

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

This comment will be updated on new commits.

@@ -428,6 +428,11 @@ class MooseVariableData : public MooseVariableDataBase<OutputType>
*/
const MooseArray<ADReal> & adDofValues() const;

/**
* Return the AD time derivative values
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Return the AD time derivative values
* Return the AD time derivative values of degrees of freedom

else
{
if (_c_nodal)
mooseError("AD neighbor nodal dof dot implemented");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mooseError("AD neighbor nodal dof dot implemented");
mooseError("AD neighbor nodal dof dot not implemented");

Comment on lines +1807 to +1823
if (_need_ad_u_dot)
{
if (_time_integrator && _time_integrator->dt())
{
_ad_dofs_dot[i] = _ad_dof_values[i];
_time_integrator->computeADTimeDerivatives(_ad_dofs_dot[i],
_dof_indices[i],
_need_ad_u_dotdot ? _ad_dofs_dotdot[i]
: _ad_real_dummy);
}
// Executing something with a time derivative at initial should not put a NaN
else if (_time_integrator && !_time_integrator->dt())
_ad_dofs_dot[i] = 0;
else
mooseError("AD nodal time derivatives not implemented for variables without a time "
"integrator (auxiliary variables)");
}
Copy link
Member

Choose a reason for hiding this comment

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

computeNodalValues is the only caller of fetchADDoFValues. I've always thought we should sync the names better. As it is, I think it's a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I ll rename!

InputParameters
CoupledAux::validParams()
CoupledAuxTempl<is_ad>::validParams()
Copy link
Member

Choose a reason for hiding this comment

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

It seems like these changes are really only for testing purposes? Is there any reason in production to use an ADCoupledAux?

Copy link
Member

Choose a reason for hiding this comment

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

And I see that you are modifying a test material ... so do we really need these aux kernel changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the changes didn't overlap but they might have ended up overlapping. I ll check

No reason to use ADcoupledAux in production. If they don't overlap do you prefer creating a new object in test/ over templating in the framework ?

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

Successfully merging this pull request may close these issues.

Central time integrator lags derivatives for AD nodal values
3 participants