-
Notifications
You must be signed in to change notification settings - Fork 6
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
Enable filter on data.format #309
Conversation
"data.spec.columns", | ||
"fmu.realization.parameters" | ||
] | ||
"exclude": ["data.spec.columns", "fmu.realization.parameters"], |
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.
Not sure what happened here. Autoformat?
@@ -134,6 +144,7 @@ def _add_filter( | |||
self, | |||
name: Union[str, List[str], bool] = None, | |||
tagname: Union[str, List[str], bool] = None, | |||
dataformat: Union[str, List[str], bool] = None, |
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.
Using dataformat
rather than format
to avoid overwriting built-in format
.
@@ -228,6 +228,9 @@ def test_case_surfaces_filter(test_case: Case): | |||
assert surf.iteration == "iter-0" | |||
|
|||
# filter on name | |||
non_valid_name_surfs = real_surfs.filter(name="___not_valid") |
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.
Added test for filter on name
@@ -236,6 +239,9 @@ def test_case_surfaces_filter(test_case: Case): | |||
assert surf.name == "Valysar Fm." | |||
|
|||
# filter on tagname | |||
non_valid_tagname_surfs = real_surfs.filter(tagname="___not_valid") | |||
assert len(non_valid_tagname_surfs) == 0 | |||
|
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.
Added test for filter on tagname
Don't know if this is the best way of solving this, inputs appreciated 💬 |
docs/explorer.rst
Outdated
@@ -279,7 +280,8 @@ Example: get aggregated surfaces | |||
We can get list of filter values for the following properties: | |||
|
|||
* names | |||
* tagnames | |||
* tagnames | |||
* dataformat |
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.
Should this be dataformats
?
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.
Yes, fixed ✅
docs/explorer.rst
Outdated
@@ -305,6 +307,7 @@ Once we have a `Surface` object we can get surface metadata using properties: | |||
print(surfaces.uuid) | |||
print(surfaces.name) | |||
print(surfaces.tagname) | |||
print(surfaces.dataformat) |
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.
Should this be dataformats
?
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.
No, but all the "surfaces" should be "surface". Fixed ✅
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.
LGTM (modulo dataformat
vs dataformats
in the documentation).
For some reason I missed that the Edit:
|
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.
👍
Issue
#304
Short description
dataformat
to the filter options, so that filtering is possible ondata.format
..dataformat
property on theChild
class, change.format
into alias for.dataformat
_We may consider deprecating
.format
at some point, to avoid guiding people into overwriting built-informat
.This may work as a mitigation for equinor/fmu-dataio#492. However, note:
An alternative would be to default to
data.format="irap_binary"
in the explorer, but this does not feel right.Extra: Also improved testing of some other filters (assert that no results come back when filtering on non-existing value)
Pre-review checklist
print()
statements, commented-out code, or other remnants from the development. 👀Pre-merge checklist