Skip to content
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

flow_ebos: Expects RTEMPVD for run without TEMP? #1231

Closed
vkip opened this issue Jun 23, 2017 · 11 comments
Closed

flow_ebos: Expects RTEMPVD for run without TEMP? #1231

vkip opened this issue Jun 23, 2017 · 11 comments

Comments

@vkip
Copy link
Member

vkip commented Jun 23, 2017

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
Copy link
Contributor

andlaus commented Jun 24, 2017

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
Copy link
Member

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
Copy link
Contributor

andlaus commented Jun 24, 2017

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
Copy link
Member

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

I agree.

@joakim-hove
Copy link
Member

joakim-hove commented Jun 24, 2017

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
Copy link
Contributor

andlaus commented Jun 24, 2017

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
Copy link
Member

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
Copy link
Contributor

andlaus commented Jun 26, 2017

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
Copy link
Member

atgeirr commented Jun 26, 2017

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
Copy link
Member

OK - this should fix it: OPM/opm-parser#1098

@blattms
Copy link
Member

blattms commented Jan 28, 2022

Now flow_ebos anymore. Closing

@blattms blattms closed this as completed Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants