temperature as function of V and p for nonideal Eq of State#3093
temperature as function of V and p for nonideal Eq of State#3093DanielDoehring wants to merge 19 commits into
temperature as function of V and p for nonideal Eq of State#3093Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
temperature as function of V and ptemperature as function of V and p for nonideal Eq of State
jlchan
left a comment
There was a problem hiding this comment.
Thanks for adding this!
We can make it non-breaking by having temperature assume V, e variables (or forward to temperature_given_Ve and noting that in the docstring. I think this is preferrable since it would keep dispatch on the specialized implementations of temperature for specific EOS. What do you think?
I think this is a good idea - although it might look a bit strange that we then forward something like |
Co-authored-by: Jesse Chan <1156048+jlchan@users.noreply.github.com>
jlchan
left a comment
There was a problem hiding this comment.
Thanks for adding the overload and removing the "breaking" label! Just two minor notes and one question.
Co-authored-by: Jesse Chan <1156048+jlchan@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3093 +/- ##
==========================================
+ Coverage 96.88% 96.89% +0.01%
==========================================
Files 652 652
Lines 50179 50139 -40
==========================================
- Hits 48615 48579 -36
+ Misses 1564 1560 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ranocha
left a comment
There was a problem hiding this comment.
Modulo the discussion on exporting the new function, this looks good to me if you also add a NEWS.md entry. Thanks a lot!
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
See #3079 (comment)
Unfortunately, this is breaking as we export
temperature.