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(UnstructuredGrid): unstructured grid from specification file #1524

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

wpbonelli
Copy link
Member

@wpbonelli wpbonelli commented Aug 30, 2022

Adapted from the notebook uploaded by @javgs-bd in #1436:

Plus a few fixes for #1520 and #1521 and other miscellaneous:

  • mention support for flopy.utils.geometry.LineString and shapely.geometry.LineString in cross section notebook
  • validate cross-section line in CrossSectionPlot instead of GeoSpatialUtil (it seems non-trivial to cover all combinations of shapetype and line representation and keep this consistent with shapely — maybe best to let shapely handle validation?)
  • group related import methods together in StructuredGrid and UnstructuredGrid
  • fix flopy.utils.gridutil.get_lni docstring
  • simplify some logic in GeoSpatialUtil tests

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #1524 (9ba92f6) into develop (d8f8dd0) will increase coverage by 0.0%.
The diff coverage is 91.3%.

❗ Current head 9ba92f6 differs from pull request most recent head a06436b. Consider uploading reports for the commit a06436b to get more accurate results

@@           Coverage Diff           @@
##           develop   #1524   +/-   ##
=======================================
  Coverage     72.4%   72.5%           
=======================================
  Files          251     251           
  Lines        54183   54227   +44     
=======================================
+ Hits         39280   39320   +40     
- Misses       14903   14907    +4     
Impacted Files Coverage Δ
flopy/utils/gridutil.py 90.9% <ø> (ø)
flopy/utils/geospatial_utils.py 89.8% <66.6%> (-1.8%) ⬇️
flopy/discretization/unstructuredgrid.py 83.5% <100.0%> (+1.4%) ⬆️

@wpbonelli wpbonelli changed the title docs: mfusg models with gsf files docs: mfusg model with gsf file notebook Aug 30, 2022
@wpbonelli wpbonelli changed the title docs: mfusg model with gsf file notebook docs: add mfusg freyberg with gsf file notebook Aug 31, 2022
@wpbonelli wpbonelli changed the title docs: add mfusg freyberg with gsf file notebook docs(examples): add mfusg freyberg with gsf file notebook Aug 31, 2022
@wpbonelli wpbonelli force-pushed the add-mfusg-notebook branch 4 times, most recently from fe42b36 to 85b01e1 Compare September 1, 2022 22:24
@wpbonelli wpbonelli marked this pull request as ready for review September 1, 2022 23:35
Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

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

I have a couple comments on this PR.

It's great that we can read a grid specification file and create a modelgrid. We already have a class method for this in the StructuredGrid class, but only for structured grids, I think. Should we do the same for the UnstructuredGrid class and add this as a class method instead (or in addition to)?

In your notebook you use copy tree to move the example files and then you load and run the model. This works just fine, but I think we have a somewhat consistent pattern where we load the model, change the workspace (m.change_model_ws(new_folder) for MF2005-based models and sim.set_simpath(new_folder) for MF6) and then run the model in the new space. Maybe we should try to be consistent in how we do this across our autotests and notebooks?

Just as an FYI, the official format for the unstructured grid specification file is described here in the gwutil_a.pdf file as:

image

@wpbonelli wpbonelli marked this pull request as draft September 2, 2022 13:12
@javgs-bd
Copy link

javgs-bd commented Sep 2, 2022

Hi, just as a note, I checked with J.Doherty and the description of the ivertex variable in that document might be a bit confusing.
The word "node" there should be replaced by "vertex".
It follows obviously from the variable name, but John was going to update the docs at some point.

@wpbonelli wpbonelli force-pushed the add-mfusg-notebook branch 5 times, most recently from 82471ce to f5ebd2e Compare September 8, 2022 17:22
@wpbonelli wpbonelli changed the title docs(examples): add mfusg freyberg with gsf file notebook feat(UnstructuredGrid): unstructured grid from specification file Sep 8, 2022
* add from_gridspec method to UnstructuredGrid
* add freyberg USG example notebook
* update cross section plot example notebook with accepted line representations
* validate line representations in PlotCrossSection instead of GeoSpatialUtil
* fix flopy.utils.gridutil.get_lni docstring
@wpbonelli
Copy link
Member Author

wpbonelli commented Sep 8, 2022

@langevin-usgs

I updated the Freyberg USG notebook to follow the load/switch workspace/run pattern. Also moved the GSF-loading functionality to UnstructuredGrid.from_gridspec to match the method on StructuredGrid. Thanks for the pointer to the specification.

I noticed that nodes defined in examples/data/freyberg_usg/freyberg.usg.gsf aren't guaranteed to have flat horizontal faces (i.e. they don't generally have 2 sets of 4 vertices, within which set each vertex shares the same elevation). The document is not explicit about this — a MODFLOW USG grid specification seems to have no concept of top/bottom elevation like UnstructuredGrid does. Is this right?

As a tentative solution UnstructuredGrid.from_gridspec sets each node's top/bottom elevation as the max/min z-value, respectively, of its constituent vertices. The implementation from the original notebook used the elevation of the first and last vertex listed for the node — I wasn't sure if it was safe to assume top vertices came before bottom, especially if top/bottom aren't part of mfusg's conceptual model.

@langevin-usgs
Copy link
Contributor

Hey @w-bonelli, I think your approach for handling the vertex issue is reasonable for now. MODFLOW-USG requires cells to have flat tops and bottoms, just like in MODFLOW 6. MODFLOW-USG reads in cell tops and bottoms as its input. MODFLOW-USG doesn't read GSF files as input; my understanding is that the GSF files are used to plot the results of MODFLOW-USG models using programs like Paraview, which can handle the x,y,z vertices. Not sure where that leaves us here, but I suspect we haven't heard the last of this. Ideally we would retain these x,y,z vertices and make them available elsewhere, like the vtk export, but I don't think we should get into that right now.

@wpbonelli wpbonelli marked this pull request as ready for review September 8, 2022 19:35
@langevin-usgs langevin-usgs merged commit af42e78 into modflowpy:develop Sep 8, 2022
@wpbonelli wpbonelli deleted the add-mfusg-notebook branch September 12, 2022 14:04
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* add from_gridspec method to UnstructuredGrid
* add freyberg USG example notebook
* update cross section plot example notebook with accepted line representations
* validate line representations in PlotCrossSection instead of GeoSpatialUtil
* fix flopy.utils.gridutil.get_lni docstring
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* add from_gridspec method to UnstructuredGrid
* add freyberg USG example notebook
* update cross section plot example notebook with accepted line representations
* validate line representations in PlotCrossSection instead of GeoSpatialUtil
* fix flopy.utils.gridutil.get_lni docstring
wpbonelli added a commit that referenced this pull request Dec 14, 2022
* add from_gridspec method to UnstructuredGrid
* add freyberg USG example notebook
* update cross section plot example notebook with accepted line representations
* validate line representations in PlotCrossSection instead of GeoSpatialUtil
* fix flopy.utils.gridutil.get_lni docstring
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* add from_gridspec method to UnstructuredGrid
* add freyberg USG example notebook
* update cross section plot example notebook with accepted line representations
* validate line representations in PlotCrossSection instead of GeoSpatialUtil
* fix flopy.utils.gridutil.get_lni docstring
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