OPM / opm-simulators

Simulator programs and utilities for automatic differentiation.
http://www.opm-project.org
GNU General Public License v3.0
111 stars 121 forks source link

flow_ebos: Expects RTEMPVD for run without TEMP? #1231

Closed vkip closed 2 years ago

vkip commented 7 years ago

flow_ebos throws exception "No such table collection: RTEMPVD" on case with no TEMP keyword. Data-file with 1D example attached.

Best regards, -Vegard. A.DATA.txt

andlaus commented 7 years ago

thanks for the report. at first glance this seems to be a bug in opm-parser since the exception is triggered when trying to access the initial reservoir temperature here. as far as I can see, that code is correct because non-existing grid properties are supposed to pop-up as soon as they are accessed for the first time. I'll have a closer look on Monday.

joakim-hove commented 7 years ago

since the exception is triggered when trying to access the initial reservoir temperature here.

Well - I have not seen the real traceback; but I think the exception is raised in the tablemanager. The opmi application parses the deck and creates a EclipseState object, so this might be a missing TableManager::hasTables( "RTEMPVD") protection?

andlaus commented 7 years ago

to me it looks like the TEMPI grid propery wants to initialize itself using RTEMPVD, but this table is not present. maybe it should simply fall back to use a constant temperature in this case. the question is: which?

joakim-hove commented 7 years ago

to me it looks like the TEMPI wants to initialize itself using the TEMPVD table, but this is not present.

I agree.

joakim-hove commented 7 years ago

OK - I agree with @andlaus analysis of what happens here:

  1. In opm-material there is a call getDoubleGridProperty("TEMPI")
  2. The grid property code does not have TEMPI property internalized and set's out to create one.
  3. The TEMPI initializer requires the RTEMPVD table - that is also not present in the deck - and now it is exception time.

It is not really clear to me how to fix this, and I hope other's will voice their opinion as well. Broadly speaking one can argue both a fix should be applied in opm-parser, and that a fix should be applied downstream:

Arguments in favor of fixing opm-parser: The GridProperty api is designed around a auto-creation behavior; if you ask for a property which is not present in the deck, the property manager should create it and initialize it with default values. Following the principles of this api through the code in opm-material should be perfectly safe, and opm-parser should be fixed.

The not-yet-explicit assumption behind the current API is that it is always possible to autocreate a property, now we have for the first time(?) experienced a situation where that is not is the case.

Arguments in favor of fixing downstream: The TEMPI keyword is an ECLIPSE300 keyword, i.e. it is not the main behavior we are targeting in flow. According to the documentation:

  1. The TEMPI keyword is for simulations where temperature variation is allowed, since temperature variation is not included in the current case it is not really sensical to ask for the TEMPI property - but of course the api encourages that kind of use.

  2. According to the manual there is no default value defined for the TEMPI keyword - i.e. there is no way that opm-parser can return a valid TEMPI property in the current situation.

All in all my suggestion is:

  1. The GridProperties<T> object is extended with a bool canCreate(const std::string& ) method - which can be used to query whether the EclipseStateobject has all the information required to create a particular property; then this check can be used instead of hasKeyword( ).

  2. Downstream must check with hasKeyword("TEMPI") or the not-yet-existing canCreate("TEMPI")before accessing the TEMPI

andlaus commented 7 years ago

I've read the documentation a bit more closely: I think we can chicken out via the RTEMP keyword (which specifies a constant reservoir temperature). the problem with this is that its default seems to be inconsistent: 60°F for E100 and 100°C for E300. we should probably go for the E100 default.

joakim-hove commented 7 years ago

I think we can chicken out via the RTEMP keyword (which specifies a constant reservoir temperature)

OK - I see that we can get out that way; but is it essential for the downstream code to get a TEMPIfield at all, in the case of temperature independent simulations?

andlaus commented 7 years ago

but is it essential for the downstream code to get a TEMPIfield at all, in the case of temperature independent simulations?

I'd say no, but yes: although nothing depends on temperature in the plain blackoil case, it is quite ackward to speak of a "thermodynamic state" if there is no temperature. I'll make a pull request for this in the afternoon.

atgeirr commented 7 years ago

I do not see why TEMPI initialization required RTEMPVD rather than RTEMP? I would guess it should use whichever is present in the deck, and if none are present it should go with RTEMP which has a default. So unless I misunderstood that I support going with "init from default RTEMP" in this case. Then we maintain the auto-creating API, which is easy to deal with from downstream.

The split defaults seem strange though! As long as we make sure to document our choice we should not worry too much about it.

joakim-hove commented 7 years ago

OK - this should fix it: https://github.com/OPM/opm-parser/pull/1098

blattms commented 2 years ago

Now flow_ebos anymore. Closing