Skip to content

TN and Advection Fixes - #33

Open
pdmullen wants to merge 6 commits into
mainfrom
pdmullen/tn-fixes
Open

pdmullen wants to merge 6 commits into
mainfrom
pdmullen/tn-fixes

Conversation

@pdmullen

@pdmullen pdmullen commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

PR Summary

TN SharedSources

  • Primitives were read from the wrong register. SharedSources receives TN's dudt subset as both sourcein and sourceout. That subset holds only Metadata::Independent vars. The associated primitives q (cm::ionization_zbar, cm::equivalent_plastic_strain, prim.<scalar>, bhr_*, …) are Metadata::Derived, so they aren't in it. GetAssociatedVars(sourceout, …) then threw Couldn't find variable for any TN run that also had ionization, strength, Mix or material-tied scalars. The var names are now resolved from state, and q is packed from state.
  • Self-associated vars got a spurious source. For Metadata::Advected vars whose "primitive" is the var itself (levelset, bulk scalars, electron_internal_energy, electron_entropy), reading the "primitive" from state would give dU += U·dρ. TN now assumes that mass-weighted vars (ρq) are associated with a separate primitive q, and that self-associated vars need no TN dρ source. The new RiotUtils::GetPrimitiveAssociatedVars keeps only the pairs with a separate primitive, and only those vars get q·dρ.

TN CalculateTNBurnSource

  • Not every source was zeroed. Only ccbulk::total_material_energy, ccmat::iso, ccmat::tn_reaction_density and ccmat::rho were zeroed, but SharedSources also accumulates into ccbulk::momentum and the anonymous conserved vars, so those built up across stages and steps. The whole TN dudt subset (all Metadata::Independent vars) is now zeroed. ccbulk::cell_delta is Metadata::OneCopy and not Metadata::Independent, so it isn't touched.
  • Identical-reactant rate is now halved. A ½ factor is applied when r_id_1 == r_id_2 (e.g. D+D), which corrects double counting in n²⟨σv⟩. Both reactant updates still take from that isotope, so two particles are consumed per reaction.

TN reaction-count history

  • IntegratedReactionCount used the wrong index. It was passed the per-material reaction index, but ccmat::tn_reaction_density is indexed by global reaction ID. Its component offset was also wrong: it summed num_reactions_per_mat over ccmat::rho components, which overcounts with phases, when each TN material actually carries num_reactions components. It now finds the material's first ccmat::tn_reaction_density component and offsets by the global reaction index.

Advection: dense vars that use the association machinery

  • Bug: dense (non-sparse) Metadata::Advected vars associated with a separate primitive (Mix's rho_bhr_*, rho_reynolds_stress) were treated as anonymous scalars. Their flux was q_upwind · riemann_vel, which is missing the density.
  • Fix: the new PrimFluxPack::HasAssociatedPrimitive detects these vars. AdvectionFluxes now multiplies the upwinded primitive by the bulk mass flux, the sum of the ccmat::rho face fluxes over all materials and phases: F(ρq) = q_upwind · Σ_m F_ρ,m. That matches how sparse material-tied vars use their own material's mass flux. Self-associated vars still use riemann_vel. Mix results will change.

Levelsets: scratch-field metadata

  • levelset0 and dudt_reinitialize were Metadata::Independent + Metadata::OneCopy. That put them in every package's dudt subset, and because they're Metadata::OneCopy the subset shares the state's memory instead of allocating new storage, so UpdateToNextStage added each var to itself once per package, and TN's new zeroing would have wiped them. Both are scratch for the operator-split Levelsets::Reinitialize and are rewritten before they're read, so they're now Metadata::Derived + Metadata::OneCopy. This also drops them from restart output and AMR prolongation/restriction. The docs table in levelsets.rst is updated.

*PR summary aided by generative AI.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the scripts/format.sh command or by using @par-hermes format
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • LANL employees: make sure tests pass both on the github CI and on the re-git CI
  • If ML was used, make sure to add a disclaimer at the top of a file indicating ML was used to assist in generating the file.
  • If Agentic AI was used, have the AI generate a "proposed changes" markdown file and store it in the plan_histories folder, with a filename the same as the MR number.

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.

Comment thread src/tnburn/shared_sources.cpp Outdated
@pdmullen pdmullen changed the title TN fixes TN and Advection Fixes Oct 7, 2026
@jdolence

jdolence commented Oct 7, 2026

Copy link
Copy Markdown
Member

Can you describe what was wrong and what the solution is?

@pdmullen pdmullen changed the title TN and Advection Fixes WIP: TN and Advection Fixes Oct 7, 2026
@pdmullen pdmullen changed the title WIP: TN and Advection Fixes TN and Advection Fixes Oct 7, 2026

@Yurlungur Yurlungur left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@pdmullen given our discussion this morning can you also create an issue regarding the longer-term refactor of anonymous advection to clarify/formalize how it should work.

Comment on lines +87 to +101
//----------------------------------------------------------------------------------------
//! \fn VarNamePairList RiotUtils::GetPrimitiveAssociatedVars
//! \brief
VarNamePairList GetPrimitiveAssociatedVars(MeshData<Real> *md,
const Metadata::FlagCollection &flags) {
auto [flagged_vars, assoc_vars] = GetAssociatedVars(md, flags);
std::vector<std::string> conserved_vars, prims_vars;
for (int n = 0; n < flagged_vars.size(); n++) {
if (flagged_vars[n] != assoc_vars[n]) {
conserved_vars.push_back(flagged_vars[n]);
prims_vars.push_back(assoc_vars[n]);
}
}
return std::make_pair(conserved_vars, prims_vars);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

given our discussion this morning is this still the way you want to do it? if so that's fine.

@Yurlungur

Copy link
Copy Markdown
Collaborator

@pdmullen given our discussion this morning can you also create an issue regarding the longer-term refactor of anonymous advection to clarify/formalize how it should work.

Oops you already did. #34

@Yurlungur Yurlungur added the bug Something isn't working label Oct 9, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants