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

fix(gridintersect): gridintersect does not work for rotated vertex grids #2107

Conversation

dbrakenhoff
Copy link
Contributor

This PR fixes #2106.

  • Gridintersect now correctly picks up the rotated/offset real-world coordinates for vertex grids.
  • Added a local kwarg that uses local model coordinates to build intersection grid if set to True.
  • Added tests

When looking through the documentation of the Grid objects I came across some entries under Attributes and Methods that do not seem to be used:

  • xgrid, ygrid and zgrid are not valid attributes as far as I can tell
  • xyzgrid is not an attribute and only available as key in Grid._cache_dict
  • the method xyvertices also doesn't exist

I removed them now in this PR, but if this is best fixed/improved in a separate PR, let me know.

- add local kwarg to force gridintersect to use local model coordinates
- update docstring
- formatting
- add tests for rotated vertexgrids and test local kwarg.
@dbrakenhoff dbrakenhoff linked an issue Feb 15, 2024 that may be closed by this pull request
@dbrakenhoff dbrakenhoff self-assigned this Feb 15, 2024
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (626563a) 73.0% compared to head (46cfea7) 73.0%.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #2107     +/-   ##
=========================================
- Coverage     73.0%   73.0%   -0.1%     
=========================================
  Files          259     259             
  Lines        59415   59396     -19     
=========================================
- Hits         43401   43386     -15     
+ Misses       16014   16010      -4     
Files Coverage Δ
flopy/discretization/grid.py 76.0% <ø> (ø)
flopy/utils/gridintersect.py 68.5% <77.7%> (-0.1%) ⬇️

... and 4 files with indirect coverage changes

@dbrakenhoff dbrakenhoff changed the title 2106 bug gridintersect does not work for rotated vertex grids fix(gridintersect): gridintersect does not work for rotated vertex grids Feb 15, 2024
Copy link
Member

@wpbonelli wpbonelli left a comment

Choose a reason for hiding this comment

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

if I understand #2106 this will change the existing behavior, I wonder if it is safer if local defaults true for now, and switches to false with the next major release?

@dbrakenhoff
Copy link
Contributor Author

You're right that this will change the default, but only for VertexGrids. The structured grids methods dealt with the rotated offset grids correctly.

So to keep current behavior I can set local to None by default and distinguish between the following routes:

  • method="vertex" forces local to True unless explicitly set (with a warning)
  • method="structured" defaults to False

In the next major release we can set local=False and remove the warning for method="vertex".

Would that be a good solution?

@wpbonelli
Copy link
Member

Since defaulting false makes structured and vertex grids consistent I see your case now to consider it a fix not a breaking change and do it now. And then we don't have to remember to switch with flopy 4

@wpbonelli wpbonelli merged commit 40c0391 into develop Feb 20, 2024
47 checks passed
@dbrakenhoff dbrakenhoff deleted the 2106-bug-gridintersect-does-not-work-for-rotated-vertex-grids branch February 20, 2024 09:30
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.

bug: gridintersect does not work for rotated vertex grids
2 participants