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
Fix Bug in PTE Solver and Several Code Quality Improvements #383
Fix Bug in PTE Solver and Several Code Quality Improvements #383
Changes from 61 commits
ef88154
8b99c75
896c205
f60e4b4
f501b2a
9cb3627
2afe06e
ca3d16a
a853cbc
2c9ecac
e885113
71250a5
a4e1f0c
5e947cd
9b3f900
3538048
1916267
bb61c27
1e2952b
52de0c4
ac21c03
6dd5411
bf28497
f017752
f913f97
d834c84
724639e
33269a4
ae40f6a
b994fdf
af9e22a
ddb1969
1c5d5b7
21a67ad
65788e5
49fbd27
0974b11
aab092a
1a048ad
92cde1c
3a67f5b
b03cb44
843057c
ae688b8
50edd22
10fb97a
a9edf92
8e89d45
a27eecb
a825f1e
5e89dd3
14a5303
73b6ea4
200531e
592c2d3
b68f524
e59140f
53f4198
e9db000
e9dbf6e
3adea33
f32ca90
4588d76
7d35c43
fd07bdd
6de9259
5910eab
8a8ac4e
a3c4d6e
9666c32
454039a
ff35cea
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.
Is this only for ideal gas? Because non-ideal gas EOS's can have non-positive energies and it's totally expected/fine.
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 next line is
This would imply a negative temperature guess, so no, this code cannot admit a negative energy density. I think this is probably part of why it's not useful outside of gas mixtures.
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 ok. I just want to make sure this is only used for ideal gas, as tabulated EOSs definitely support negative 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.
yep! I think this is a good reason not to use this guess for tabular EOS. You could also imagine a reacting system where the reaction products is represented by a shifted ideal gas with a negative energy. I think this guess isn't really used, but I figured it might be good to put guardrails on it.
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 function is called in the
Init
forPTESolverRhoU
which I believe is supposed to handle non-ideal EOS.Notice that
TryIdealPTE
is commented out fromPTESolverRhoT
with the following comment: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 unresolved this conversation because I was concerned that
PTESolverRhoU
now no longer works for non-ideal EOS wherein u can be less than 0. Any thoughts here?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 suggest that we floor the energy/temperature inside the solver, as it's only used for guesses
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.
If
PTESolverRhoU
relies on a negative temperature guess, I'd argue that it never worked for non-ideal EOS that can have negative energies and that the guess infrastructure should be re-worked for that solver.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 I misread the above. I forgot that
TryIdealPTE()
is separate fromGetIdealPTE()
. So these error checks being located inGetIdealPTE()
is a mistake sinceTryIdealPTE()
will check the results ofGetIdealPTE()
and see if the results are sane.Even though these changes are highlighted just because I changed the message and not because they were added here, I'll move them to the
TryIdealPTE()
function.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.
Actually, I ended up getting rid of all the
GetIdealPTE
asserts and instead I added a check inTryIdealPTE()
to make sure the temperature guess is actually positive.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.
@rbberger do you have a preference on a different way to do this? I.e. how do I identify a new dependency in a version that hasn't been released yet?
Or should we handle these at the release and just set them to
when="@main"
for now?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.
@cmauney @Yurlungur @dholladay00 do you have preferences here?
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.
@rbberger any thoughts?