feat(flopy plugins) #1746
feat(flopy plugins) #1746
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1746 +/- ##
=========================================
- Coverage 72.1% 71.7% -0.4%
=========================================
Files 253 262 +9
Lines 56086 57252 +1166
=========================================
+ Hits 40452 41097 +645
- Misses 15634 16155 +521
|
There was a problem hiding this comment.
Hi @spaulins-usgs, lots of really great stuff here. This is some tremendous new functionality. I tried to go through it all and make some comments. I added quite a few, but hopefully they will be helpful. Happy to discuss further.
| @@ -200,6 +200,36 @@ Contents: | |||
| ./source/flopy.mf6.utils.reference.rst | |||
| ./source/flopy.mf6.utils.lakpak_utils.rst | |||
|
|
|||
| FloPy for MODFLOW 6 Plugins | |||
There was a problem hiding this comment.
From what I can tell, this section is for a plugin developer. What about a plugin user? Presumably these instructions do not apply, and installing a flopy plugin will automatically make it available.
Also, are these instructions up to date with the recent changes you've made for plugin development?
There was a problem hiding this comment.
This section is referring to the plugin_interface.py and plugin_template.py files, which are for the plugin developer, not the user. However, I agree that there should be a section here for the plugin user. I therefore added a section describing how to use FloPy plugins and reference the documentation of the “package interface” files of the RVC and RVP plugins (the two plugins included with FloPy) to help the user get started.
The instructions are up to date with the recent changes I made.
.docs/code.rst
Outdated
| change the behavior of an existing MODFLOW 6 package or behave like a new | ||
| stand-alone MODFLOW 6 package. There are three ways FloPy can detect plugins. |
There was a problem hiding this comment.
| change the behavior of an existing MODFLOW 6 package or behave like a new | |
| stand-alone MODFLOW 6 package. There are three ways FloPy can detect plugins. | |
| change the behavior of an existing MODFLOW 6 package or create a new | |
| stand-alone MODFLOW 6 package. There are three ways FloPy can detect plugins. |
| "source": [ | ||
| "## Working with flopy_npc_plugin.py\n", | ||
| "\n", | ||
| "The plugin template generator creates and modifies several files. The plugin code file that you will edit in order to add functionality to your plugin is in the in the python working folder (folder containing this notebook) and in this case is named \"flopy_npc_plugin.py\". This file contains a class derived from FPBMIPluginInterface with several methods. \n", |
There was a problem hiding this comment.
| "The plugin template generator creates and modifies several files. The plugin code file that you will edit in order to add functionality to your plugin is in the in the python working folder (folder containing this notebook) and in this case is named \"flopy_npc_plugin.py\". This file contains a class derived from FPBMIPluginInterface with several methods. \n", | |
| "The plugin template generator creates and modifies several files. The plugin code file that you will edit in order to add functionality to your plugin is in the python working folder (folder containing this notebook) and in this case is named \"flopy_npc_plugin.py\". This file contains a class derived from FPBMIPluginInterface with several methods. \n", |
| "source": [ | ||
| "## Working with flopy_npc_plugin.py\n", | ||
| "\n", | ||
| "The plugin template generator creates and modifies several files. The plugin code file that you will edit in order to add functionality to your plugin is in the in the python working folder (folder containing this notebook) and in this case is named \"flopy_npc_plugin.py\". This file contains a class derived from FPBMIPluginInterface with several methods. \n", |
There was a problem hiding this comment.
Does the name of this folder really have a .py extension?
There was a problem hiding this comment.
That is the file name not the folder name. For clarity I reworded the sentence describing this.
| "id": "0c0c0f91", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "Next define any user-specified settings for your plugin. These settings will exist in a \"package\" file similar to the files used to store MODFLOW-6 package user-specified settings. The FloPy plugin template generator can set up an options, package, and stress period block for your \"package\" settings file.\n", |
There was a problem hiding this comment.
| "Next define any user-specified settings for your plugin. These settings will exist in a \"package\" file similar to the files used to store MODFLOW-6 package user-specified settings. The FloPy plugin template generator can set up an options, package, and stress period block for your \"package\" settings file.\n", | |
| "Next define any input that will be provided by the user of the plugin. This input will be provided by the user in a \"package\" input file similar to the MODFLOW 6 files provided as input to a regular MODFLOW 6 simulation. The FloPy plugin template generator can set up an options, package, and stress period block for your \"package\" settings file.\n", |
flopy/mf6/modflow/mfsimulation.py
Outdated
There was a problem hiding this comment.
It seems like there is a lot of new flopy plugin code that was added here. Would it be possible to move this into a plugin folder somewhere as most of it is not needed for the dominant regular flopy use case.
There was a problem hiding this comment.
Cleaned this up moving ~90% of the new code in MFSimulation into a new “plugin_runner.py” file.
| return cell_id[0] + 1 | ||
|
|
||
| @staticmethod | ||
| def sq_saturation(top, bot, x, c1=None, c2=None): |
There was a problem hiding this comment.
Maybe there is good reason for this, but this does not seem like a good place for replicated low-level GWF NPF calculation routines.
There was a problem hiding this comment.
This method was included because I originally found it useful for some plugins I was writing. Since nothing in this PR currently uses this method, I just removed it.
| return y | ||
|
|
||
| @staticmethod | ||
| def sq_saturation_derivative(top, bot, x, c1=None, c2=None): |
There was a problem hiding this comment.
same comment as before
There was a problem hiding this comment.
Removed, as commented above.
| Package data retrieval methods | ||
| """ | ||
|
|
||
| def mf6_idomain_val(self, cell_id): |
There was a problem hiding this comment.
Seems dangerous to have all these hard-wired routines here.
There was a problem hiding this comment.
I am not sure I understand why these properties are dangerous. They are just there to make it easier for the FloPy plugin developer to get common grid geometry information from FloPy. If you do not think these are appropriate here, I can remove them. Maybe I can add a utility method class for these and the low-level GWF NPF calculations?
There was a problem hiding this comment.
Are we including these plugins in flopy now? Or should we make separate repos for them and require users to install them as plugins? I think requiring them to be installed would offer some nice benefits and would also require us to think about the structure of flopy plugin repos.
There was a problem hiding this comment.
I would like to include these plugins in flopy now. I think it is nice to have some easily accessible plugin examples with FloPy, and the autotest code I added to test FloPy plugins uses these plugins to test this feature. I am all for minimizing the effort for users who want to just quickly try out FloPy plugins and see what it is all about. I agree that we should talk about the structure of flopy plugin repos and if/how we are going to support identifying a list of available (unsupported) plugins.
There was a problem hiding this comment.
Not really convinced that the name os this should FloPy Plugins. This is not general use plugin code that allows any user to create python packages that modifies the behavior of FloPy. Instead this is plugin code that allows users to develop python packages and api callback containers for modflow 6 that then can be used as plugins with flopy.
For example, with this implementation I couldn't develop specialized plugin code that could handle reading modflow-owhm or modify pygsflow to be an optional plugin/extension for flopy. Some general plugin hooks to make things like this possible would be really beneficial. I could imagine in the future we could then have something like flopy[core] (which is the core gwf and gwt functionality), and as modflow 6 grows additional supported components (e.g. support for the surface water model being developed) could be installed as plugins to core functionality. This is similar to how "ray" packages their different functionalities.
The actual plugin code is really buried in flopy.mf6.utils.plugins, is there a better location for this and if we develop generalized plugin hooks too, we should think about where it should live?
I've left some comments where I think the code could be simplified or flopy is duplicating work that is already being done by modflowapi.
Overall, the Modflow 6 plugin system you've developed seems really useful and I think it'll be a nice addition to flopy and modflowapi! Let me know if you have any questions about my review comments.
| slnobj = list(mf6_sim.solutions.values())[0] | ||
| converged = ( | ||
| mf6_sim.allow_convergence and mf6_sim.iteration < slnobj.mxiter | ||
| ) |
There was a problem hiding this comment.
I don't think flopy should be checking convergence status. The modflowapi runner already does this, and this code block would break the support for ATS that's in the modflowapi.
| mf6_sim.allow_convergence = ( | ||
| mf6_sim.allow_convergence and allow_cvg | ||
| ) |
There was a problem hiding this comment.
I don't think flopy should have a seperate allow convergence flag. Instead it should directly set the ApiSolution object's allow_convergence attribute.
| class FPPluginInterface: | ||
| """ | ||
| Base class of flopy plugins for MODFLOW 6. Your flopy | ||
| plugins should not directly inherent from this class, inherent from | ||
| the FPAPIPluginInterface instead. | ||
|
|
||
| Attributes | ||
| ---------- | ||
| simulation : MFSimulation | ||
| Simulation object that this plugin is a part of. Simulation object | ||
| acquired when simulation is run by flopy and the receive_vars | ||
| method is called. | ||
| model : MFModel | ||
| Model object that this plugin is a part of. | ||
| package : MFPackage | ||
| Package data that the flopy plugin for MODFLOW 6 uses to determine | ||
| the plugin's user settings. | ||
| mg : Grid | ||
| Model grid object for the model this plugin is a part of. | ||
|
|
||
| """ |
There was a problem hiding this comment.
Is there a good reason for this to be a parent class? It's currently only inherited by FPAPIPluginInterface
| def val_from_cellid(self, cell_id, grid_array): | ||
| """This method retrieves a value from an array in the shape of the | ||
| model grid for a particular cell id. Structured, vertex and | ||
| unstructured grids are supported. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| cell_id : tuple | ||
| Cell ID corresponding to the location in the array where a | ||
| value should be retrieved. | ||
| grid_array: numpy ndarray | ||
| Array containing value to be retrieved. |
There was a problem hiding this comment.
grid_array naming convention could be a little confusing. It implies to me that it's an array of grid data.
| elif self.mg.grid_type == "vertex": | ||
| assert len(cell_id) == 2 | ||
| return self.mg.get_node([cell_id])[0] |
There was a problem hiding this comment.
vertex model grids do not currently have a get_node() method. This could either be added to the VertexGrid class or calculated here as [cellid[0] * ncpl] + cellid[1].
It would be great to maintain support for passing in lists or ndarrays of cellids that the mg.get_node function already has.
There was a problem hiding this comment.
Same formatting comment as listed with the other notebooks
| FloPy plugins are python code designed to change the behavior of MODFLOW 6. | ||
| Using FloPy plugins is similar to using a MODFLOW 6 package in FloPy. Two | ||
| example plugins are included with FloPy which behave similar to the RIV | ||
| package, but allow for different conductances in the upward and downward | ||
| directions. To use FloPy plugins instantiate a plugin input class | ||
| similar to the "package" input classes FloPy uses to add a package to an | ||
| existing model. Documentation for the plugin input classes of the two | ||
| example plugins are included below. |
There was a problem hiding this comment.
I'm not entirely convinced that these plugins should be included in the flopy repo. It might be better to have the example plugins adjacent and have users install them, so they get used to working with plugins.
Additionally, they are the same calculation that is done with two different approaches: 1) Using the API package, and 2) overriding the functionality of the RIV package.
There was a problem hiding this comment.
Like Josh, I am still uncertain if this functionality should be in flopy.
| ModflowGwffp_Rvp defines a fp_rvp package that is a flopy plugin extension | ||
| of a gwf6 model. |
There was a problem hiding this comment.
Is there a way to generate better docstings? This does not give any information about what the RVP package is used for.
| ModflowGwffp_Rvc defines a fp_rvc package that is a flopy plugin extension | ||
| of a gwf6 model. |
There was a problem hiding this comment.
Is there a way to generate better docstings? This doesn't give any information about what the rvc package does or is used for
| _notebooks/tutorial01_mf6_plugin | ||
| _notebooks/tutorial02_mf6_plugin |
There was a problem hiding this comment.
It'd be nice to have a tutorial on how to override functionality in an existing package. The current set of tutorials shows how to use an existing plugin and how to develop one in the API package. Modifying existing package behavior is an important topic.
|
@jlarsen-usgs, these are great comments. Lots to consider. Makes me wonder if flopy is the right place to put these "plugins". They might be more aptly named plugins if they were included in modflowapi instead. That way the "plugin" term in flopy could be leveraged for the uses cases that you mention. |
There was a problem hiding this comment.
There is alot here to digest. I am still not convinced this belongs in flopy. It belongs somewhere but maybe not in flopy.
I like Josh's idea that a flopy plugin would extend flopy functionality. I could see a flopy plugin providing input and output support for MODFLOW-OWHM or MODFLOW-USG. That way it would be up to the person developing the plugin to support it and flopy could support what we want it to. We could think about a version of flopy (flopy4) that only supports mf6 and base functionality (modelgrid, geoproocessing, plotting, etc) and move old MODFLOW, MT3D, SEAWAT, etc functionality out as a plugin (this is an idea Chris brought up).
What you are calling a "flopy plugin" is providing a pre-canned way to use the API that seems like flopy. Josh's additions to the modflowapi package already helps alot in allowing users to interact with the API in a flopy way. So maybe "flopy plugins" should just be additional helper functionality in the modflowapi package.
I think more thought needs to be given to naming. Function names and variables should provide users with an unambiguous idea of what is going on.
I don't like that flopy dfns are in the repo. It seems like an opportunity for confusion over who owns dfns.
I am not completely clear on the difference between the rvc and rvp examples. They do seem overly complicated. I think one of the cases should be using a standard river package with an aux variable that defines either the inflow or outflow conductance and the code picks the appropriate one depending on the flow direction. Maybe I missed something.
| ] | ||
|
|
||
| api = [ |
There was a problem hiding this comment.
why has a separate api block been included in the toml? This should just be in the existing optional block. The standard modflowapi release should be used not the repo.
| FloPy plugins are python code designed to change the behavior of MODFLOW 6. | ||
| Using FloPy plugins is similar to using a MODFLOW 6 package in FloPy. Two | ||
| example plugins are included with FloPy which behave similar to the RIV | ||
| package, but allow for different conductances in the upward and downward | ||
| directions. To use FloPy plugins instantiate a plugin input class | ||
| similar to the "package" input classes FloPy uses to add a package to an | ||
| existing model. Documentation for the plugin input classes of the two | ||
| example plugins are included below. |
There was a problem hiding this comment.
Like Josh, I am still uncertain if this functionality should be in flopy.
| is_flopy_package = ( | ||
| self.structure.name == "packages" | ||
| and arr_line_len > 1 | ||
| and arr_line[1].startswith("FP_") |
There was a problem hiding this comment.
Might be better to use PI_ or better yet plugin_ to be clearer. Here and throughout the code.
| @@ -0,0 +1,97 @@ | |||
| from flopy.mf6.utils.flopy_plugins import FPAPIPluginInterface | |||
There was a problem hiding this comment.
FPAPIPluginInterface is a terrible name. PluginInterface would be better.
| import numpy as np | ||
|
|
||
|
|
||
| class FPPluginInterface: |
There was a problem hiding this comment.
FPAPIPluginInterface is a terrible name. PluginInterface would be better
| """ | ||
| return True | ||
|
|
||
| def val_from_cellid(self, cell_id, grid_array): |
There was a problem hiding this comment.
Looks like this returns a single value. If true couldn't this function be value_from_cellid().
grid_array can just be array or arr
Why would it ever be another modelgrid.grid_type? If you want to provide protection it should issue an error if it another grid_type.
| return f"{self.lib_name}.dylib" | ||
| return "" | ||
|
|
||
| def get_api_dll(self): |
|
|
||
| Returns | ||
| -------- | ||
| success : bool | ||
| buff : list of lines of stdout | ||
|
|
||
| """ | ||
| if debug_mode: | ||
| fd_debug = open("debug_run_sim.txt", "w") |
There was a problem hiding this comment.
I think we need to think about if we want flopy to open a separate file for writing information. Users can always specify print_input and print_budget to have information written to the mf6 lst file.
|
I am closing this PR since Flopy Plugins (now Flopy API Plugins) is now going to be its own library. |
FloPy plugins are FloPy-integrated components that modify MODFLOW-6 functionality using the modflowapi library. The FloPy plugins architecture streamlines the process of modifying MODFLOW-6 functionality by: