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(check): Updated flopy's check to work with cellid -1 values #1885

Merged
merged 7 commits into from
Aug 6, 2023
Merged

fix(check): Updated flopy's check to work with cellid -1 values #1885

merged 7 commits into from
Aug 6, 2023

Conversation

spaulins-usgs
Copy link
Contributor

  • Check now accepts -1 values for cellid, though enforces that if one cellid value is -1 all of them must be -1
  • Fixed a problem where an incomplete grid is returned in MF6 when idomain is none. For MF6 idomain now defaults to and array 1's, as described in the MF6 documentation
  • Fixed a potential problem for vertex grids where both based class and child class initializing _top, _botm, and _idomain
  • Fixed a couple of test/example scripts, where the updated code above caught invalid cellids

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #1885 (a9abc35) into develop (8c330a7) will decrease coverage by 0.1%.
The diff coverage is 59.3%.

@@            Coverage Diff            @@
##           develop   #1885     +/-   ##
=========================================
- Coverage     72.2%   72.2%   -0.1%     
=========================================
  Files          257     257             
  Lines        56414   56441     +27     
=========================================
+ Hits         40751   40765     +14     
- Misses       15663   15676     +13     
Files Changed Coverage Δ
flopy/discretization/vertexgrid.py 82.1% <ø> (-0.6%) ⬇️
flopy/mf6/data/mfdatalist.py 71.6% <25.0%> (-0.7%) ⬇️
flopy/mf6/mfmodel.py 59.0% <70.8%> (+0.3%) ⬆️

... and 2 files with indirect coverage changes

@staticmethod
def _resolve_idomain(idomain, botm):
if idomain is None:
if botm is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand why there is a dependence of idomain on botm. Do we want to retain idomain=None if botm is not defined. Wouldn't it be better from the modelgrid creation to fail if botm has not been defined when creating a discretization instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our current approach with modelgrid creation is to allow for the creation of a modelgrid without top and botm. This has always been allowed so that a user can make use of much of the modelgrid functionality without using top and botm. Also, a modelgrid is requested by flopy whenever grid data is loaded and the "check" option requested, including when top and botm are loaded. Failing to create modelgrid when botm is still in the process of loading would break our current approach. I am open to reconsidering this approach, but I thought that was beyond the scope of this PR, which is to fix a very specific problem.

Regarding the dependence of initializing the default idomain (of all 1's) on botm, to initialize the default idomain one needs to first know the shape of the model grid. The modelgrid class currently has only one variable, besides idomain, that defines the full shape of the grid, botm. The simplest, and hopefully least prone to error, implementation was therefore to get the shape of idomain from the botm array.

However, on further considering there is a potential problem with this. The model grid could get cached with the default idomain values when flopy loads botm and then not get updated when idomain is finally loaded. Going to add some code to fix this potential bug.

@jdhughes-usgs jdhughes-usgs merged commit 68f0c3b into modflowpy:develop Aug 6, 2023
21 checks passed
wpbonelli pushed a commit that referenced this pull request Aug 25, 2023
* fix(test): should be a complete grid
* fix(model grid): Always resync mf2005 grids
* Revert "fix(model grid): Always resync mf2005 grids"
* fix(grid idomain): grid idomain now defaults to all 1's for MF6 only
* fix(notebook): notebook contained out of bounds cellid
* fix(modelgrid): always force modelgrid to resync until an idomain array is provided

---------

Co-authored-by: scottrp <45947939+scottrp@users.noreply.github.com>
@wpbonelli wpbonelli mentioned this pull request Aug 25, 2023
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.

3 participants