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

Simulation metadata #323

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Simulation metadata #323

merged 1 commit into from
Apr 28, 2020

Conversation

asnyv
Copy link
Collaborator

@asnyv asnyv commented Apr 27, 2020

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

@asnyv asnyv force-pushed the metadata branch 2 times, most recently from 7ec5f7e to 48afc8c Compare April 28, 2020 06:20
Copy link
Collaborator

@anders-kiaer anders-kiaer left a 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.

Comment on lines 556 to 557
def get_unit(smry_meta, vec):
return "" if smry_meta is None else simulation_unit_reformat(smry_meta.unit[vec])
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@asnyv
Copy link
Collaborator Author

asnyv commented Apr 28, 2020

equinor/resdata#707 merged upstream

@asnyv asnyv force-pushed the metadata branch 2 times, most recently from 3fa323a to ff9ddc0 Compare April 28, 2020 13:08
@asnyv asnyv merged commit 52eccc0 into equinor:master Apr 28, 2020
@asnyv asnyv deleted the metadata branch June 24, 2020 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Utilize metadata from fmu-ensemble
2 participants