-
Notifications
You must be signed in to change notification settings - Fork 59
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
Simulation metadata #323
Simulation metadata #323
Conversation
7ec5f7e
to
48afc8c
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.
Nice work 🎉 Overall looks good. Some questions/suggesitons below.
webviz_subsurface/plugins/_reservoir_simulation_timeseries_onebyone.py
Outdated
Show resolved
Hide resolved
def get_unit(smry_meta, vec): | ||
return "" if smry_meta is None else simulation_unit_reformat(smry_meta.unit[vec]) |
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.
Would it be simpler/better that this returns None
when there is no unit, instead of empty string?
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.
It is probably "better", and was considering it, but empty string is "safer" in the sense that no errors are thrown if you don't include None
handling each place the get_unit
is used. Quite easy to change it still, so I can if you want me to.
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.
Vote for changing to None
. Then I guess it would also be easy to distinguish between missing/undefined unit, and a unit-less quantity/ratio?
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.
👍 will do. And yes, that should be easy.
equinor/resdata#707 merged upstream |
3fa323a
to
ff9ddc0
Compare
Resolves #284.
The "more robust vector vs node name split" was not done, as it seems like
fmu_ensemble
/libecl
doesn't support all the properties of vector names that we use (especially for region keywords). Think our existing solution is the best we could hope for atm.Some improvements have also been made to the vector type identification (
is_rate
/is_total
) upstream in libecl. but the code is not dependent on these changes.Will also improve the
is_historical
flag when this issue is completed . Should be pretty straight forward as long as we get the ideas "approved".Since a lot of the modifications were related to vectors and units, I tried to keep #296 in the back of my head. Point 1 and 2 in #296 is therefore solved in this PR.
Also extended list of "human readable units" from the Eclipse
METRIC
unit format