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

feat(lgr-disv): add to_disv_gridprops() method to lgr object #2271

Merged
merged 10 commits into from
Jul 26, 2024

Conversation

langevin-usgs
Copy link
Contributor

@langevin-usgs langevin-usgs commented Jul 23, 2024

Add to_disv_gridprops() method to the Lgr object. The lgrutil.Lgr class helps to create the parent and child configuration for a two-model modflow6 simulation. The new method combines the parent and child grids into a single DISV mesh. Once this mesh is created, it can be used to create a single-model modflow6 simulation that should give the same results as the two-model parent-child simulation.

Most of the work of this export involves the identification and handling of "hanging vertices." Hanging vertices are vertices that must be added along the edges of parent cells to mark the locations of child cell corners. These vertices are not strictly necessary to define a parent cell grid shape (a square or rectangle can be defined by just the four corner vertices), but they are required by modflow so that connections between parent and child cells can be automatically determined through shared vertices.

There are some limited nested-grid capabilities available in cvfdutil.gridlist_to_disv_gridprops() but they do not seem to work for refinement contrasts greater than 1 parent to 3 child cells (as identified in #2263). The new method as part of this PR also avoids geometric intersections, which could be problematic with the cvfdutil version, and instead determines hanging vertices directly through parent-child relations, essentially using a form of integer math.

For the problem identified in #2263, we now get the correct hanging vertices for cellid 11, as shown in the figure below.

image

One small limitation with the current approach is that there may be duplicate vertices, but only the first will ever be used. It may be beneficial to write a compression function for verts and iverts to remove any vertices that are not used. This could be generally useful, and might live in cvfdutil.

@langevin-usgs langevin-usgs changed the title feat(lgr-disv): add new to_disv_gridprops to lgr object feat(lgr-disv): add to_disv_gridprops() method to lgr object Jul 23, 2024
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 96.02273% with 7 lines in your changes missing coverage. Please review.

Project coverage is 74.3%. Comparing base (71fbd26) to head (8361fbc).
Report is 7 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #2271     +/-   ##
=========================================
+ Coverage     73.7%   74.3%   +0.5%     
=========================================
  Files          294     294             
  Lines        59102   59365    +263     
=========================================
+ Hits         43593   44139    +546     
+ Misses       15509   15226    -283     
Files Coverage Δ
flopy/utils/lgrutil.py 97.1% <96.0%> (+10.9%) ⬆️

... and 20 files with indirect coverage changes

Copy link
Contributor

@dbrakenhoff dbrakenhoff left a comment

Choose a reason for hiding this comment

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

Thanks @langevin-usgs, I tried using the LGR class to set up the grids and I can confirm that it's working nicely!

One comment, about the function gridlist_to_disv_gridprops. This function hasn't been changed so it will not work for a refinement level beyond 3. What are the plans for this function? Is it just a utility function for some of the flopy tests or is it exposed to users? If the latter option, it would be smart to either add a trap, or fix the function to work for an arbitrary refinement level. What do you think?

@langevin-usgs
Copy link
Contributor Author

Thanks @langevin-usgs, I tried using the LGR class to set up the grids and I can confirm that it's working nicely!

One comment, about the function gridlist_to_disv_gridprops. This function hasn't been changed so it will not work for a refinement level beyond 3. What are the plans for this function? Is it just a utility function for some of the flopy tests or is it exposed to users? If the latter option, it would be smart to either add a trap, or fix the function to work for an arbitrary refinement level. What do you think?

Thanks for looking, @dbrakenhoff. I'm inclined to deprecate and then remove gridlist_to_disv_gridprops. The new capability in the lgr utility is much more powerful, I think. If there was opposition to deprecating the functionality, then an alternative would be to rewrite gridlist_to_disv_gridprops to simply instantiate an lgr utility object behind the scenes and return a corresponding disv mesh. The current coding in gridlist_to_disv_gridprops seems a little convoluted, and I was thinking it may be best to ditch it. Any thoughts?

@dbrakenhoff
Copy link
Contributor

I doubt many people were using the utility, so deprecation sounds like the way to go then I think!

@langevin-usgs langevin-usgs merged commit 7dec7c5 into modflowpy:develop Jul 26, 2024
26 checks passed
@langevin-usgs langevin-usgs deleted the feat-lgr-todisv branch August 8, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants