Deprecate Implicit gridprop init #662

eivindjahren · 2021-10-07T12:29:13Z

closes #646

src/xtgeo/grid3d/_ecl_logi_head.py

jcrivenaes

Most looks good. It is a big PR so it could be good if one other can review, if available?

src/xtgeo/grid3d/_ecl_output_file.py

dafeda · 2021-11-16T13:44:49Z

src/xtgeo/grid3d/_gridprop_import_eclrun.py

+    return init_props[0]
+
+
+def import_gridprop_from_restart(


I think this function could benefit from type-hints, docstring and perhaps a test.

Do I read this right that date can be of some kind of format like "2012-26-07", and "20122607" and one of "all", "first", "last"?
If so, why?

Yes, date can be any one of those, and the reason is to avoid breaking changes. I'll add the same typehints from the other functions.

Added typehint.

Also, "all" is not a valid option, so removed that.

"all" still seems to be an option for both find_gridprops_from_restart_file and find_gridprops_from_restart_file_sections since typehints Literal["all"] are used.
Are you thinking about something else?

By the way, Literal[] is new to me and it's cool so thanks :)

Yes, "all" is still an option for "find". I think the confusion is that the "find" functions are reused to also find more than one gridproperty.

dafeda · 2021-11-16T13:59:10Z

src/xtgeo/grid3d/_gridprop_import_eclrun.py

@@ -568,3 +565,35 @@ def import_gridprops_from_restart_file(
    if close:
        filehandle.close()
    return read_properties
+
+
+def import_gridprop_from_init(pfile, name, grid, fracture=False):


I find it a bit hard to parse that import_gridprop_from_init calls import_gridprop_from_init_file.
Do we need both?

This is indeed some bad naming, we need both as we will need to have an entry point which does the collecting both when we are getting just one grid property and when we are getting a collection of properties matching a search criteria. I guess some wording involving search or collect might help.

I tried a rename to find in case that helps.

dafeda · 2021-11-16T14:08:29Z

src/xtgeo/grid3d/_gridprop_import_xtgcpprop.py

@@ -14,7 +14,7 @@
 logger = xtg.functionlogger(__name__)


-def import_xtgcpprop(self, mfile, ijrange=None, zerobased=False):
+def import_xtgcpprop(mfile, ijrange=None, zerobased=False):


Docstring outdated - self can be removed.

dafeda · 2021-11-16T14:09:17Z

src/xtgeo/grid3d/_gridprop_import_xtgcpprop.py

@@ -25,7 +25,6 @@ def import_xtgcpprop(self, mfile, ijrange=None, zerobased=False):
        zerobased (bool): If ijrange basis is zero or one.

    """
-    #
    offset = 36


Why not 42? :)

That is indeed the question

dafeda · 2021-11-16T15:09:16Z

src/xtgeo/grid3d/_gridprop_roxapi.py

+            result["values"].reshape(-1).compressed(), roxprop.code_names
+        )
+        logger.info("Fixed codes: %s", result["codes"])
+    return result


 def export_prop_roxapi(


Remove self-parameter.

Same goes for the function _store_in_roxar in the same file.

This is the export functions so they do indeed take an object instance. Whether it should be named self is another thing. I think that renaming is out of scope for this PR, but could be made a separate issue.

dafeda · 2021-11-17T06:43:07Z

src/xtgeo/grid3d/_gridprop_import_eclrun.py

@@ -474,7 +471,7 @@ def section_generator(generator):
            return


-def import_gridprops_from_restart_file_sections(
+def find_gridprops_from_restart_file_sections(


Pedantic alert (!)

"Imports list of parameters from an sections generator."

"Finds list of parameters from a sections generator."

dafeda · 2021-11-17T07:47:39Z

src/xtgeo/grid3d/_gridprop_import_eclrun.py

+
+
+def sanitize_date(
+    date: Union[int, str, Literal["first", "last"]]


The way I read this type is that we allow any int and any string and that mypy would not complain if we passed say "norway" as the date parameter.

What if we specified Union[int, Literal["first", "last"]] and have the caller be responsible for converting a string representation of a date to int?

That is unfortunately not possible now as what we are doing here is sanitizing input from the public API where str has been allowed. So dropping it would be a breaking change. We can make sure the "find" part of the code does not need to worry about str though. I can create an issue for that as it means bigger changes.

dafeda

I think this is a nice improvement.
In general, the functions could use a bit more testing, typing and docstrings, but hey can't fix it all one PR!

Deprecates initialization of GridProperty object from file, and non-zero default values in initialization.

eivindjahren force-pushed the explicit_gridprop_init branch 4 times, most recently from 402804f to be443eb Compare October 29, 2021 11:31

eivindjahren changed the title ~~Explicit gridprop init~~ Deprecate Implicit gridprop init Oct 29, 2021

eivindjahren marked this pull request as ready for review October 29, 2021 11:40

eivindjahren force-pushed the explicit_gridprop_init branch from be443eb to 774ea0b Compare October 29, 2021 11:56

jcrivenaes reviewed Nov 8, 2021

View reviewed changes

src/xtgeo/grid3d/_ecl_logi_head.py Outdated Show resolved Hide resolved

jcrivenaes reviewed Nov 8, 2021

View reviewed changes

src/xtgeo/grid3d/_ecl_output_file.py Outdated Show resolved Hide resolved

src/xtgeo/grid3d/_ecl_output_file.py Outdated Show resolved Hide resolved

src/xtgeo/grid3d/_ecl_output_file.py Outdated Show resolved Hide resolved

eivindjahren force-pushed the explicit_gridprop_init branch 3 times, most recently from 6ea7b3a to fd14516 Compare November 10, 2021 06:09

eivindjahren mentioned this pull request Nov 11, 2021

Refactor gridprop from ecl run #664

Merged

eivindjahren force-pushed the explicit_gridprop_init branch 3 times, most recently from ca1fa7b to 6b9aeee Compare November 12, 2021 08:21

equinor deleted a comment from ertomatic Nov 12, 2021

eivindjahren force-pushed the explicit_gridprop_init branch 5 times, most recently from 3e20010 to aec1f55 Compare November 16, 2021 09:37

dafeda self-requested a review November 16, 2021 10:13

dafeda reviewed Nov 16, 2021

View reviewed changes

eivindjahren force-pushed the explicit_gridprop_init branch 2 times, most recently from 93274cb to 8942580 Compare November 16, 2021 20:13

eivindjahren force-pushed the explicit_gridprop_init branch from 8942580 to 0ccf427 Compare November 17, 2021 06:12

eivindjahren requested review from dafeda and jcrivenaes November 17, 2021 06:14

eivindjahren force-pushed the explicit_gridprop_init branch from 0ccf427 to e7d00aa Compare November 17, 2021 06:18

dafeda reviewed Nov 17, 2021

View reviewed changes

eivindjahren force-pushed the explicit_gridprop_init branch from b13f581 to 24f9d98 Compare November 17, 2021 07:16

dafeda reviewed Nov 17, 2021

View reviewed changes

dafeda approved these changes Nov 17, 2021

View reviewed changes

eivindjahren and others added 2 commits November 17, 2021 11:58

ENH: Deprecated implicit init of gridproperty

0f2432a

Deprecates initialization of GridProperty object from file, and non-zero default values in initialization.

CLN: Clean up file opening in find_gridprops

c18a779

eivindjahren force-pushed the explicit_gridprop_init branch from 372356a to c18a779 Compare November 17, 2021 10:58

eivindjahren merged commit 37515d6 into equinor:master Nov 17, 2021

eivindjahren deleted the explicit_gridprop_init branch November 17, 2021 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate Implicit gridprop init #662

Deprecate Implicit gridprop init #662

eivindjahren commented Oct 7, 2021 •

edited

Loading

jcrivenaes left a comment

dafeda Nov 16, 2021

eivindjahren Nov 16, 2021 •

edited

Loading

eivindjahren Nov 16, 2021

eivindjahren Nov 16, 2021

dafeda Nov 17, 2021

eivindjahren Nov 17, 2021

dafeda Nov 16, 2021

eivindjahren Nov 16, 2021 •

edited

Loading

eivindjahren Nov 16, 2021 •

edited

Loading

dafeda Nov 16, 2021

eivindjahren Nov 16, 2021

dafeda Nov 16, 2021

eivindjahren Nov 16, 2021

dafeda Nov 16, 2021 •

edited

Loading

eivindjahren Nov 16, 2021 •

edited

Loading

dafeda Nov 17, 2021

eivindjahren Nov 17, 2021

dafeda Nov 17, 2021

eivindjahren Nov 17, 2021 •

edited

Loading

dafeda left a comment



		def sanitize_date(
		date: Union[int, str, Literal["first", "last"]]

Deprecate Implicit gridprop init #662

Deprecate Implicit gridprop init #662

Conversation

eivindjahren commented Oct 7, 2021 • edited Loading

jcrivenaes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eivindjahren Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eivindjahren Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

eivindjahren Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dafeda Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

eivindjahren Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eivindjahren Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

dafeda left a comment

Choose a reason for hiding this comment

eivindjahren commented Oct 7, 2021 •

edited

Loading

eivindjahren Nov 16, 2021 •

edited

Loading

eivindjahren Nov 16, 2021 •

edited

Loading

eivindjahren Nov 16, 2021 •

edited

Loading

dafeda Nov 16, 2021 •

edited

Loading

eivindjahren Nov 16, 2021 •

edited

Loading

eivindjahren Nov 17, 2021 •

edited

Loading