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

Update the implementation to identify region-to-region variables #551

Merged
merged 1 commit into from
Jan 8, 2019

Conversation

joakim-hove
Copy link
Contributor

@joakim-hove joakim-hove commented Dec 19, 2018

The type of the summary variables is inferred by looking at the string, e.g. all variables like WWCT and WOPT which start with W are well variables.

Variables starting with 'R' are region properties, but for region properties there is a sub-category of variables which describe region-to-region properties. This PR updates the code to identify region-to-region properties. Previously the algorithm to detect a region to region variable was just to check the third character - and all variables with var[2] == 'F'were classified as region to region variables.

This PR will change the classification in two ways:

  1. It is stricter - in addition to third character we also check the fourth character - and require that var[3] == 'T' || var[3] == 'R'.

  2. It is more lenient - we also check for the substring FT or FR one character further out in the string - i.e. starting at the fourth character in var.

@ghost ghost added the in progress label Dec 19, 2018
@joakim-hove
Copy link
Contributor Author

An error message from Jenkins would have been very much appreciated ...

@joakim-hove
Copy link
Contributor Author

There is clearly an issue with this - would be very grateful if you could post the error message from the failing test(s).

@markusdregi
Copy link
Contributor

Sorry for the delayed responses. I'm still catching up after Christmas.

The easy answer (for me) is that this is the output from Jenkins:

114/275 Test #118: ecl_region2region ........................................................***Failed 0.59 sec
/tmp/tmp.ki8DPBweDc/libecl/lib/ecl/tests/ecl_region2region.cpp:27 => assert( true ) failed

However, I don't know how helpful that is.

@joakim-hove
Copy link
Contributor Author

OK; now the tests pass - which is a good thing. This PR updates the region-2-region classification according to the manual, that I am certain is correct - however the internal tests explicitly tested the old region - 2 - region detection algorithm; and therefor the old detection is also retained. This is explained in the comment in the code; the current PR is my "best shot" - but tricky...

Copy link
Contributor

@lars-petter-hauge lars-petter-hauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with the changes in the pr 👍

@joakim-hove joakim-hove merged commit 08272b1 into equinor:master Jan 8, 2019
@ghost ghost removed the in progress label Jan 8, 2019
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 this pull request may close these issues.

None yet

3 participants