-
Notifications
You must be signed in to change notification settings - Fork 32
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
Wsegaicd support #201
Wsegaicd support #201
Conversation
ecl2df/common.py
Outdated
@@ -54,6 +54,8 @@ | |||
"WELOPEN", | |||
"WELSEGS", | |||
"WELSPECS", | |||
"WSEGSICD", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make list alphabetical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if "KEYWORD_IDX" in compdat_df.columns: | ||
compdat_df.drop(["KEYWORD_IDX"], axis=1, inplace=True) | ||
|
||
return dict(COMPDAT=compdat_df, COMPSEGS=compsegs_df, WELSEGS=welsegs_df) | ||
if "KEYWORD_IDX" in wsegsicd_df.columns: | ||
wsegsicd_df.drop(["KEYWORD_IDX"], axis=1, inplace=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this drop is not done for WELSEGS and COMPSEGS, for which WSEGxICD are analogues. So potentially it should not be done for WSEx*ICD or it should also be done for WELSEGS and COMPSEGS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, her har det skjedd noe feil. La meg sjekke litt og gjøre en pull-request til.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, WSEGxICD is analogous to COMPDAT with only one record, so I used COMPDAT as a template. WELSEGS and COMPSEGS have two records. KEYWORD_IDX is not defined in WELSEGS and COMPSEGS and therefore not dumped. If KEYWORD_IDX should have been defined/dumped for WELSEGS and COMPSEGS I think you should discuss with the developer for those keywords?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that sounds reasonable. Maybe that information should be included as a code comment.
tests/test_compdat.py
Outdated
assert compdat_df["WELL"].unique()[0] == "OP_6" | ||
|
||
# Check that we have not used the very long opm.io term here: | ||
assert "CONNECTION_TRANSMISSIBILITY_FACTOR" not in compdat_df |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should not be needed here, as it is tested further up?
Can you shorten the tests here to only those that are not tested elsewhere?
(test code also requires future maintenance, and superfluous test code increases maintenance burden)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ja, fjerner alt som er gjort før men legger inn en test for WSEGSICD i tillegg til WSEGAICD som ligger der fra tidligere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! Thanks for the contribution
Conflicts: tests/test_compdat.py
* Added support for WSEGAICD and WSEGSICD in response to issue equinor#199 Co-authored-by: Trygve Kløv (ST RRR RM) <tklov@st-linrgsn199.st.statoil.no>
Support for WSEGSICD and WSEGAICD was added in response to issue #199. Functionality is included in
-ecl2df/opmkeywords/WSEGSICD
-ecl2df/opmkeywords/WSEGAICD
Two tests are added to
New include files for testing are added to