search

OPM / opm-parser

http://www.opm-project.org
11 stars 44 forks source link

respect the fact that non-SI temperature may be offset from zero #1203

Closed andlaus closed 6 years ago

andlaus commented 6 years ago

also, the UnitTests test was buggy because it tested absolute temperature when it was supposed to test the temperature.

this should fix #1202.

andlaus commented 6 years ago

jenkins build this please

totto82 commented 6 years ago

Maybe adding the test case I provided in #1202?

andlaus commented 6 years ago

the non-absolute temperature is already tested, albeit only for PVT-M. I can still add it if there is agreement, but the benefits would not be very large?

akva2 commented 6 years ago

this broke the restart test in opm-output.

joakim-hove commented 6 years ago

Damn - my fault, sorry.

I will force push HEAD~~ and then we can repeat more carefully?

atgeirr commented 6 years ago

These commits are probably correct, maybe the restart test is quick to fix?

joakim-hove commented 6 years ago

OK - should be fixed here: https://github.com/OPM/opm-output/pull/241

I misunderstood the message from @akva2 slightly - I thought the whole test-rig was in shatters; things were not that bad.

atgeirr commented 6 years ago

Failing opm-simulators tests (on both my machine and Jenkins) are caused by this:

38: Comparing TEMP...
38: Occurrence in first file    = 0
38: Occurrence in second file   = 0
38: Grid coordinate             = (1, 1, 1)
38: (first value, second value) = (-459.666, 519.669)
atgeirr commented 6 years ago

Looking at the actual output temperatures, I think there is a significant chance that the code now has a significant bug. Consider the data above, the old output was 519.669 F, which is approx 15.5 C. I think that has been used as a default temperature, so it makes sense. The new output is -459.666 F, which is absolute zero. Clearly we still want to output the default temperature here.

The only use of the offset (outside the converter function here) I found in opm-parser is in TableManager.cpp line 98. I am not sure if it is correct or not?

joakim-hove commented 6 years ago

Ok - what a mess. I suggest to revert this and the PR in opm-material.

atgeirr commented 6 years ago

Ok - what a mess. I suggest to revert this and the PR in opm-material.

I concur.

andlaus commented 6 years ago

go ahead. to_si() and from_si() and the UnitsTest should be fixed, though?

andlaus commented 6 years ago

BTW: the old output probably was about 520°R, not °F...

joakim-hove commented 6 years ago

OK - I will now revert this and: https://github.com/OPM/opm-material/pull/282 to recover the Jenkins

We (I at least ...) will have to look more into this, and recover the parts we should retain.

atgeirr commented 6 years ago

BTW: the old output probably was about 520°R, not °F...

Yes, I meant Rankine, sorry. So there is a bug already in the current master, since we are expected to output in Fahrenheit! (So we should at the end of this give the right output in F not R for this problem).