-
Notifications
You must be signed in to change notification settings - Fork 15
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
ENH: include (fault) surfaces from FaultRoom plugin #492
Conversation
eb3df8b
to
017e44d
Compare
fbdcfde
to
fba9002
Compare
examples/s/d/nn/xcase/realization-0/iter-0/rms/bin/export_faultroom_surfaces.py
Outdated
Show resolved
Hide resolved
dff724c
to
5443629
Compare
examples/s/d/nn/xcase/realization-0/iter-0/rms/bin/export_faultroom_surfaces.py
Outdated
Show resolved
Hide resolved
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.
See comments. Not entirely sure if class: surface
is the best option, although I don't think it's "wrong". If we merge this, need to clarify with later users of these data. E.g. there may be some assumptions somewhere that class: surface
== "an IRAP Binary formatted regular surface".
- Explicitly check with REP
- Explicitly check with Sumo
- Generally check with other potential consumers
I think it was never the intention that class |
025e2f1
to
ddd5df2
Compare
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.
Looks good to me, i cant comment on the domain related questions.
Still waiting for feedback from REP, Webviz, DynaGeo and others who may work on the assumption that |
I think you need to rebase here as well before merging, else some of the schema changes will be lost. Any the poor person who checksout from main will get "schema out of sync errors". |
Any progress here? |
Some... But I don't think we have to wait for everything to be ready everywhere. I think we just merge this PR if we are happy. |
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 say we merge this.
37b979d
to
0f10f78
Compare
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.
Some comments for your consideration but LGTM. I cannot comment on the fault room specifics, of course
0f10f78
to
7827552
Compare
7827552
to
d310bbb
Compare
Address #362