Jmm/t sie fix #495
Jmm/t sie fix #495
Conversation
|
I also added a new example that can quickly allow a user to report these bounds from a spiner file. |
|
Tests pass on re-git. |
| @@ -475,16 +475,16 @@ class SpinerEOSDependsRhoSie : public EosBase<SpinerEOSDependsRhoSie> { | |||
| } | |||
| PORTABLE_FORCEINLINE_FUNCTION Real rhoMax() const { return rhoMax_; } | |||
| PORTABLE_FORCEINLINE_FUNCTION Real TMin() const { | |||
| return fromLog_(T_.range(0).min(), lTOffset_); | |||
| return fromLog_(sie_.range(0).min(), lTOffset_); | |||
There was a problem hiding this comment.
Let me make sure I follow here:
- sie = f(rho, T) and T = f(rho, sie)
- so you're getting the T bounds by:
- looking at the domain of T in sie
- the sie bounds by:
- looking at the domain of sie in T?
There was a problem hiding this comment.
and they are both rank 2 databoxes in which the 2nd dimension is density and the first is (T, sie), depending on the data box?
There was a problem hiding this comment.
Yep exactly. In SpinerEOSDependsRhoSie, the temperature is a function of density and energy and energy is a function of density and temperature. That point of confusion was the cause of the bug I think.
There was a problem hiding this comment.
and they are both rank 2 databoxes in which the 2nd dimension is density and the first is (T, sie), depending on the data box?
Yep.
| printf("rho bounds, depends(r, T): %.14e %.14e\n", deprt.MinimumDensity(), | ||
| deprt.rhoMax()); | ||
| printf("rho bounds, depends(r, sie): %.14e %.14e\n", depre.MinimumDensity(), | ||
| depre.rhoMax()); | ||
| printf("T bounds, depends(r, T): %.14e %.14e\n", deprt.MinimumTemperature(), | ||
| deprt.TMax()); | ||
| printf("T bounds, depends(r, sie): %.14e %.14e\n", depre.MinimumTemperature(), | ||
| depre.TMax()); |
There was a problem hiding this comment.
I realize this isn't necessarily touched by this MR, but I do think the naming should be consistent between min and max, i.e. MinimumTemperature and MaximumTemperature or TMin and Tmax
There was a problem hiding this comment.
Agreed. Reason it's not consistent is MaximumTemperature isn't a public function in the variant. That may be the time to fix.
PR Summary
The
MinimumTemperatureandMinimumDensityfunctions inSpinerEOSDependsRhoSiehad a bug that reported the incorrect bounds. This MR fixes this bug and adds a test. Thanks to Julien Loiseau for reporting the issue.PR Checklist
make formatcommand 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