Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Draft: copy/past boiler-plate + some changes, does not compile at the moment #422
Changes from 4 commits
c3206d7
5ebda78
c5ab4d6
8237aa3
efc1033
0e32674
331e5a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 asTnorm
(unless it isn't passed in) but it seems like we always usetemp[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 thatpress
needs to contain sane values of some sort.While I'm at it, I'll create an issue to rename
Tguess
and note thattemp
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.
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.
https://github.com/lanl/singularity-eos/blob/dholladay00/pt_solver/singularity-eos/closure/mixed_cell_models.hpp#L328
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:
Tequil = temp[0]
InitBase()
InitBase()
calculates the initial pressure fromTscale
and the input density, NOTTequil
Pequil
, which is the initial PTE stateThere 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 theneq - 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$\left(\frac{\partial P}{\partial e}\right)_V = \rho \Gamma$ derivative for the metal is likely near 10-ish, so this implies that the energy variability will be maybe 10% of the pressure variability due to volume fraction changes. If we assume the volume fraction is maybe order 1e-05, then the mass fraction might be order 1e-02, and since the energy sums with the mass fraction, I would estimate the variability to be around 1e-03 for a doubling of the pressure. The total volume fraction variability would be near zero since the bulk modulus is large.
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
andPequil - psum/vsum
checks to thecheckpte
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.
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).
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.