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

Add types to grid #958

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Add types to grid #958

merged 1 commit into from
Nov 9, 2023

Conversation

janbjorge
Copy link
Contributor

@janbjorge janbjorge commented Nov 3, 2023

Resovles #892
Resolves #894

@janbjorge janbjorge self-assigned this Nov 3, 2023
@janbjorge janbjorge marked this pull request as ready for review November 7, 2023 07:13
Copy link
Collaborator

@mferrera mferrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really superb work on a massive file and one of the most fundamental classes. I think I figured out the doc build issue, and comments about them are included on dataframe and the method directly below it.

src/xtgeo/types.py Outdated Show resolved Hide resolved
src/xtgeo/grid3d/_grid3d.py Outdated Show resolved Hide resolved
src/xtgeo/grid3d/grid.py Outdated Show resolved Hide resolved
src/xtgeo/grid3d/grid.py Show resolved Hide resolved
src/xtgeo/grid3d/grid.py Outdated Show resolved Hide resolved
src/xtgeo/grid3d/grid.py Outdated Show resolved Hide resolved
src/xtgeo/grid3d/grid.py Outdated Show resolved Hide resolved
src/xtgeo/grid3d/grid.py Outdated Show resolved Hide resolved
Comment on lines 2435 to 2431
_grid_etc1.reverse_row_axis(self, ijk_handedness=ijk_handedness)
self._tmp = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you noticed this yourself but beginning to feel there must be a better way to handle things with (or preferably without) self._tmp. Not relevant to this PR of course but just a general comment

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the self._tmp is applied in _grid_etc1.py and _grid3d_fence.py to store some data to speed up execution, but I am sure this can handled in a better way.

src/xtgeo/grid3d/grid.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2023

Codecov Report

Merging #958 (9d411ab) into main (b0aa977) will decrease coverage by 0.17%.
Report is 6 commits behind head on main.
The diff coverage is 79.60%.

@@            Coverage Diff             @@
##             main     #958      +/-   ##
==========================================
- Coverage   80.48%   80.31%   -0.17%     
==========================================
  Files          91       92       +1     
  Lines       13512    13494      -18     
  Branches     2227     2227              
==========================================
- Hits        10875    10838      -37     
- Misses       1910     1929      +19     
  Partials      727      727              
Files Coverage Δ
src/xtgeo/grid3d/_find_gridprop_in_eclrun.py 94.00% <100.00%> (-0.03%) ⬇️
src/xtgeo/grid3d/_grid3d.py 91.30% <100.00%> (+7.30%) ⬆️
src/xtgeo/grid3d/_gridprop_import_eclrun.py 84.09% <100.00%> (-0.36%) ⬇️
src/xtgeo/grid3d/_gridprops_import_eclrun.py 82.35% <100.00%> (-0.15%) ⬇️
src/xtgeo/common/types.py 0.00% <0.00%> (ø)
src/xtgeo/grid3d/grid.py 81.42% <87.42%> (-1.91%) ⬇️
src/xtgeo/common/sys.py 74.58% <70.63%> (-2.01%) ⬇️

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

src/xtgeo/types.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jcrivenaes jcrivenaes 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 👍

@janbjorge janbjorge merged commit 8f6b9be into equinor:main Nov 9, 2023
26 checks passed
@janbjorge janbjorge deleted the add-types-grid branch November 9, 2023 13:00
Copy link
Collaborator

@mferrera mferrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment about the subgrids type but I think this is good to go after that! Thanks very much for working with the comments and the great work. This is a beast of a file to take on

Comment on lines -578 to +601
def subgrids(self):
def subgrids(self) -> Optional[OrderedDict[str, range | list[int]]]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this type is still not fully correct, it should be

Optional[OrderedDict[str, range], OrderedDict[str, list[int]]] , two possible dict types not a single one with two possible values for keys. Relevant for the deprecated init, init, setter, maybe one or two others places. Then you could get rid of the # type: ignore in subgrids_from_zoneprop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add type annotations to _Grid3D
4 participants