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

Document heat transfer kernels. #29646

Merged
merged 9 commits into from
Jan 17, 2025
Merged

Conversation

aprilnovak
Copy link
Contributor

@aprilnovak aprilnovak commented Jan 5, 2025

Closes #29645

Reason

I would like to teach a class using MOOSE; improved documentation will help students.

Summary/Justification of Changes

  • The phrase "diffusivity" specifically means the ratio of the property on the Laplacian term relative to that on the time term. It is not a synonym for diffusion coefficient. Relevant documentation corrected.
  • It is a common misconception that rho * Cp are only outside the time derivative if they are constant. They should always be outside (this can be shown from a rigorous derivation of the heat equation starting from conservation of total energy (kinetic + internal), the entropy equation, and thermodynamic identities. Relevant documentation corrected.
  • I think the statement on the HeatConductionTimeDerivative page about not considering on-diagonal Jacobian contributions from Cp and rho is just wrong -- the source code shows it is there? Relevant documentation corrected.
  • I tried to use a consistent approach to have every AD page just reference the non-AD page to avoid duplicating text.

Discussion for additional ideas not yet in this PR:

  • I would like to propose deleting HeatCapacityConductionTimeDerivative. Looks like it's not used anywhere in the framework, not even tested.
  • SpecificHeatConductionTimeDerivative looks like a better version of HeatConductionTimeDerivative because it uses the DerivativeMaterialInterface to get off-diagonal Jacobian terms. Would suggest making HeatConductionTimeDerivative internally equivalent to the former and then deprecating SpecificHeatConductionTimeDerivative to remove confusion.

@aprilnovak aprilnovak requested a review from lindsayad as a code owner January 5, 2025 19:32
@moosebuild
Copy link
Contributor

Job Precheck, step Clang format on 5a012b4 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/29646/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 1e17680adfcc2117439231c38b5bb9cb3a102636

@moosebuild
Copy link
Contributor

moosebuild commented Jan 5, 2025

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

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Jan 6, 2025

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

Framework coverage

Coverage did not change

Modules coverage

Heat transfer

b45164 #29646 332ed8
Total Total +/- New
Rate 88.63% 88.63% - 100.00%
Hits 4414 4414 - 4
Misses 566 566 - 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

@GiudGiud GiudGiud self-assigned this Jan 7, 2025
@aprilnovak
Copy link
Contributor Author

I think the test failure is unrelated?

@aprilnovak
Copy link
Contributor Author

I opted to remove any discussion about Jacobians -- it's not done uniformly across the documentation, and IMO if someone is concerned about Jacobian issues they should be using AD.

Other than the comment about AD vs. non-AD markdown files, I think I've addressed everything.

@moosebuild
Copy link
Contributor

Job Coverage, step Verify coverage on 9c53c05 wanted to post the following:

The following coverage requirement(s) failed:

  • Failed to generate chemical_reactions coverage rate (required: 92.0%)

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

woops I led you astray there for a bit

@lindsayad
Copy link
Member

Looks like you still have some links to ADGenericConstantMaterial.md

@aprilnovak
Copy link
Contributor Author

ack sorry, fixed it.

@lindsayad
Copy link
Member

failure unrelated to documentation

@lindsayad lindsayad merged commit efd3dd1 into idaholab:next Jan 17, 2025
50 of 51 checks passed
@lindsayad lindsayad deleted the heat-transfer-docs branch January 17, 2025 01:02
kyriv1980 pushed a commit to loganharbour/moose that referenced this pull request Jan 17, 2025
kyriv1980 pushed a commit to loganharbour/moose that referenced this pull request Jan 17, 2025
GiudGiud pushed a commit to GiudGiud/moose that referenced this pull request Jan 22, 2025
GiudGiud pushed a commit to GiudGiud/moose that referenced this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document heat transfer kernels
4 participants