-
Notifications
You must be signed in to change notification settings - Fork 8
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
Draft: copy/past boiler-plate + some changes, does not compile at the moment #422
base: main
Are you sure you want to change the base?
Conversation
… but need to make sure where to put derivatives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! Let me know when you want further review
// calculate an initil equilibrium pressure | ||
Real psum = 0.0; | ||
Real vsum = 0.0; | ||
for (int m = 0; m < nmat; ++m) { | ||
psum += vfrac[m]*press[m]; | ||
vsum += vfrac[m]; | ||
} | ||
// all normalized temps set to 1 so no averaging necessary here. | ||
Tequil = temp[0]; | ||
Pequil = psum / vsum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This machinery has always confused me. We pass in a Tguess
that then gets saved as Tnorm
(unless it isn't passed in) but it seems like we always use temp[0]
as the initial starting point for the solver, yes?
We could take the same approach with press[0]
, but your approach does provide a reasonable guess if the pressure is evolved by the host code in some way. We will just need to note that press
needs to contain sane values of some sort.
While I'm at it, I'll create an issue to rename Tguess
and note that temp
requires at least one sane value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #423
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recall that temp
is in arbitrary normalized temperature units and so it will always be multiplied by Tnorm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we also normalize the pressures too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think they are normalized by an internal energy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with either duplicating the temperature approach (i.e. Pequil = press[0]
) or staying with this. Whatever we do though, it needs to be noted in the documentation what values need to be sane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pressures are initialized from a lookup so they are potentially different whereas there is 1 temperature guess calculated and it is folded into the temperature normalization and the temperature vector is initialized to 1 for every element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pressures are initialized from a lookup so they are potentially different
Can you point to this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, I'm with you now. We should definitely document this.
To summarize my understanding:
- The initial temperature guess for the PTE solver is
Tequil = temp[0]
- The provided pressure can be nonsense since it is populated by
InitBase()
InitBase()
calculates the initial pressure fromTscale
and the input density, NOTTequil
- You're volume fraction averaging those pressures to get
Pequil
, which is the initial PTE state
void Residual() const { | ||
Real tsum = 0.0; | ||
Real psum = 0.0; | ||
Real vsum = 0.0; | ||
// is volume averaging the right thing here? | ||
for (int m = 0; m < nmat; ++m) { | ||
tsum += vfrac[m]*temp[m]; | ||
psum += vfrac[m]*press[m]; | ||
vsum += vfrac[m]; | ||
} | ||
residual[0] = Tequil - tsum/vsum; | ||
residual[1] = Pequil - psum/vsum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth discussing possible convergence criteria, but your suggestion may end up being the best approach.
The residual equations are formulated based off of the density and energy sums, which is why they're included in the residuals for the other solvers. Of course in the PTESolverRhoT
solver, we also populate the rest of the neq - 2
entries with pressure differences, but that's because we require additional constraints in addition to the primary residual equations. Not including the energy/volume sums in the residual would mean that we're determining convergence separate from the actual residual equations themselves. This isn't necessarily bad, but we need to be intentional about it.
If we imagine a situation with a small volume of metal surrounded by gas, the pressure is highly sensitive to changes in the volume fraction of metal in the mixture. The
With your residuals, the temperature residual variability would be similar to the energy residual variability above, but the pressure residual variability would be on the order of the volume fraction, or 1e-05. So this formulation might be less sensitive to small volume fractions of stiff materials. The plus side of course is less iterations, but the downside might be a solid density inconsistent with the actual PTE pressure. I'm not sure that's very bad since small volume fractions of solid/metal are always annoying, and being robust to them is a good thing.
Maybe this is an opportunity though. Do you think you could add a switching variable to the class that switches between a traditional residual (energy and volume fraction sums) and this one you're proposing? I.e. keep what you have here but use something more like the first two entries of PTESolverRhoT
when a switch is on/off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should some of the checks you're talking about in the alternate scheme just be placed in the checkpte function? The residual function as I understand it is really about the unknowns of the system and the checkpte function exists for checking additional constraints AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it might make sense to add the Tequil - tsum/vsum
and Pequil - psum/vsum
checks to the checkpte
function since they're not the actual residual equations being solved. The information is valuable, but I'm concerned that these aren't the actual residuals of the residual equations we're solving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my reading of the document (equation 12), these exrpessions form the residual of [T* - T, P*-P].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider equation 13 to be the residual. Equation 12 arises from the linearization of the nonlinear system, and is really just an update vector for the Newton solve.
In my experience, the convergence criterion is primarily a function of some norm of the residual itself, but it can also sometimes be related to the norm of the update when satisfying the residual equations isn't as important as finding a fixed point. In this case though, I'm concerned that volume/energy conservation is more important than all the materials being at the same temperature/pressure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I'm all sorts of confused here. You are correct. I'll fix this when I get a chance.
bool CheckPTE() const { | ||
using namespace mix_params; | ||
Real mean_p = vfrac[0] * press[0]; | ||
Real error_p = 0.0; | ||
for (int m = 1; m < nmat; ++m) { | ||
mean_p += vfrac[m] * press[m]; | ||
error_p += residual[m + 1] * residual[m + 1]; | ||
} | ||
error_p = std::sqrt(error_p); | ||
Real error_u = std::abs(residual[1]); | ||
// Check for convergence | ||
bool converged_p = (error_p < pte_rel_tolerance_p * std::abs(mean_p) || | ||
error_p < pte_abs_tolerance_p); | ||
bool converged_u = (error_u < pte_rel_tolerance_e || error_u < pte_abs_tolerance_e); | ||
return converged_p && converged_u; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this code needs to be updated to reflect the structure of the residual.
My suggestion above to include two residual formulations would complicate this function (perhaps requiring two implementations of the function actually).
jacobian[0] = rtor2_dr_dT_p_sum; | ||
jacobian[1] = rtor2_dr_dP_T_sum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure the sign is correct here, but let me know if there are any issues with my math too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check again, but IIRC there is a double negative that cancels.
PR Summary
early beginnings of the pt space pte solver. It's mostly a copy-paste for now. Hopefully I can get things more cleaned up soon. Feel free to take a look @jhp-lanl
PR Checklist
make format
command after configuring withcmake
.If preparing for a new release, in addition please check the following:
when='@main'
dependencies are updated to the release version in the package.py