fix(exg-gwf*): check for equal idomain #2149
Conversation
| @@ -220,6 +228,34 @@ subroutine exg_ar(this) | |||
| call store_error(errmsg, terminate=.TRUE.) | |||
| end if | |||
| ! | |||
| ! -- Make sure idomains are identical | |||
| select type (gwfdis => gwfmodel%dis) | |||
There was a problem hiding this comment.
All the select type statements are unfortunate, but idomain is defined separately for each dis type since it's the real grid shape instead of flat
There was a problem hiding this comment.
Ahh, bummer. I guess now that all discretizations have IDOMAIN, we could move it to disbasetype and make it a 1D int vector, but that is for another time, I think.
There was a problem hiding this comment.
If I am understanding correctly, we are trying to assess the equality of grid. Maybe we can have a function is_equal(this, other_dis) result(are_equal) to delegate the check to the particular Discretization module?
There was a problem hiding this comment.
PS: happy to help and chat more about that option
There was a problem hiding this comment.
@mjr-deltares thanks we should definitely chat on this with @langevin-usgs
It may be relevant to #2153 as well. In that PR, I first planned to load the full flow model dis into fmi, then an is_equal function would be quite natural to use. But the dis types have some strong expectations about how they are initialized and where they get their data from. So in the near term, I was thinking of loading only the minimum necessary info to do the same checks. Hopefully we can come up with a nicer long term solution.
There was a problem hiding this comment.
This is a good point. In this new check, we are saying the grids are the same if the number of nodes and idomain is the same; it is easy to accidentally leave out idomain when using flopy to construct dis. But certainly, if tops and bottoms were also different, then that would not be detected here, and problems would be very difficult to find. A discretization-based is_equal() (and maybe is_same()?) seems like a nice way to ensure grid equality.
There was a problem hiding this comment.
OK, let's talk tomorrow and plan a meeting. Btw I had no intention stalling this PR. This should probably go in and then we refactor when we have a plan.
wpbonelli
changed the title
wpbonelli
requested review from
langevin-usgs,
aprovost-usgs and
emorway-usgs
Check that models connected to a flow model via an exchange have an identical idomain array to the flow model's.
wpbonelli
force-pushed
the
exg-check-idomain
branch
from
554f955 to
c3a5be7
Compare
| @@ -220,6 +228,34 @@ subroutine exg_ar(this) | |||
| call store_error(errmsg, terminate=.TRUE.) | |||
| end if | |||
| ! | |||
| ! -- Make sure idomains are identical | |||
| select type (gwfdis => gwfmodel%dis) | |||
There was a problem hiding this comment.
Ahh, bummer. I guess now that all discretizations have IDOMAIN, we could move it to disbasetype and make it a 1D int vector, but that is for another time, I think.
Support an optional GWFGRID entry in FMI for the flow model's binary grid file. If provided, check the grids have the same size and idomain like we do in EXG as of #2149. This makes for the same error-checking in the exg (coupled) and fmi (post-processing) cases. Tentative idea is to make the grid file entry optional for some time (1-2 releases), then begin to require it. At which point the reader could be modified to load the full dis, and we could consolidate these haphazard checks into a robust grid comparison. The grb file reader in zonebudget is moved to a new GridFileReader module in the main src dir. The reader now loads headers and builds an index at init time, after which variables can be read as necessary.
Check that models connected to a flow model by an exchange have an idomain identical to the flow model's idomain. This is an easy error to make, and (up to now) hard to detect.
I only added a test for PRT for now. The logic is identical in GWT and GWE. I wondered if there are suitable autotests for those models to add a test case to? Or maybe this behavior should have its own autotest?
#2144 facilitates adding the same check for flow model interface. I figure that can be done in a separate PR since it probably deserves a separate release note?
Checklist of items for pull request
fprettify