modflowusg #1246
modflowusg #1246
Conversation
|
@cnicol-gwlogic, can you help with this? |
|
@langevin-usgs Yep I can help, apologies I did not get on to it earlier. I had been thinking through a plan for usg the last few days. I'll take a look at this today. |
* move modflowdisu to modflowusg * add deprecation warning to flopy.Modflow.load() for modflowusg models * removed some modflowusg bits from flopy.Modflow; now in flopy.ModflowUsg instead * utils.gridgen updated for this refactor * several bug fixes so autotests will run * autotests t005_test, t016_test, t038_test, t061_test_gridgen, t506_test updated for this refactoring ModflowUsgLpf and ModflowUsgBcf still need refactoring / superclassing (from ModflowLpf and ModflowBcf) to minimise code duplication. Possibly ModflowUsgWel too.
|
@langevin-usgs I've refactored a fair bit of this by inheriting from the modflow LPF/BFC/WEL classes from modflowusg - hopefully @swfwmd accepts my PR swfwmd/flopy#2. The tests should all run fine then. I think this leaves it all a bit cleaner if people want to add more exotic mfusg funcitionality. Cheers. Happy to take suggestions for mods if anyone has them too. |
Codecov Report
@@ Coverage Diff @@
## develop #1246 +/- ##
=============================================
+ Coverage 73.498% 74.341% +0.843%
=============================================
Files 230 232 +2
Lines 50389 50900 +511
=============================================
+ Hits 37035 37840 +805
+ Misses 13354 13060 -294
|
|
@swfwmd and @cnicol-gwlogic, nice job with this! I like how you override the structured modflow packages. This is wayyy cleaner, and gives you full control to support additional mfusg functionality without disturbing the modflow flopy code base. Looks like there is still some work to do with the test failures, but hopefully those won't be too hard to handle. And one thing, I'd like to suggest. I see how you've shut off the add_package method for mfusg models for bcf, lpf, wel, etc. Could you make that more general by adding an add_package=True argument to those classes. Then when you call super() on those, you can set add_package=False. This keeps the add_package functionality generic and not specific to mfusg. Minor detail. |
Mfusg refactor for pylint/flake8
|
See #1238 which caused the conflicts, and introduced a pattern of simpler subclassing that should also apply to this PR. |
flopy/__init__.py
Outdated
| __all__ = [ | ||
| " __version__", | ||
| "__author__", | ||
| "__author_email__" "modflow", |
There was a problem hiding this comment.
missing a , here, and on L47
flopy/modflowusg/mfusgbcf.py
Outdated
| Overrides the parent ModflowBcf object.""" | ||
| msg = ( | ||
| "Model object must be of type flopy.modflowusg.ModflowUsg\n" | ||
| + "but received type: {type(model)}." |
There was a problem hiding this comment.
| + "but received type: {type(model)}." | |
| f"but received type: {type(model)}." |
this same edit applies 14 other times in this PR
flopy/modflowusg/mfusgcln.py
Outdated
| raise Exception("mfcln: ja_cln must be provided") | ||
| if abs(self.ja_cln[0]) != 1: | ||
| raise Exception( | ||
| "mfcln: first ja_cln entry (node 1) is " "not 1 or -1." |
There was a problem hiding this comment.
| "mfcln: first ja_cln entry (node 1) is " "not 1 or -1." | |
| "mfcln: first ja_cln entry (node 1) is not 1 or -1." |
flopy/modflowusg/mfusgcln.py
Outdated
| self.filenames.append(None) | ||
|
|
||
| # Fill namefile items | ||
| name = [ModflowUsgCln._ftype()] |
There was a problem hiding this comment.
Within class methods like __init__(self, ..., this can simply be self._ftype(). Classmethods like load(cls, ...) it can be cls._ftype(). There ~17 "._ftype()" matches here that could potentially be simplified.
refactor(mfusg) undo some refactoring
|
Looks like there are some merge conflicts and codacy issues with this PR. I think it's getting close, though. Thanks @swfwmd, @cnicol-gwlogic, and @mwtoews. Not sure of the best path forward here. If you think the mfusg changes are clean and almost ready, then perhaps it's better to rebase or try fresh? The merged changes from other PRs are diluting the changes mfusg here. |
|
Hi @langevin-usgs thanks and apologies, it's gettting a bit painful (and messy). Two questions on codacy: 1) is it only the critical issues that cause a fail? And 2) can I replicate those tests locally so I can check before I push? I tried running flake8 and pylint using the two commands from ci.yml but they give different outcomes. But yes, might be time for a fresh PR. Ta. |
|
So codacy seems to be overdoing things at the moment. I think you should try and fix what it finds, if possible. But ultimately, when you get to a point where you think the PR is ready to go, just let us know, and we can overlook minor codacy complaints. I would imagine there is a way to run codacy locally, but I'm not aware of how we would do it. We will probably have to have a discussion on our end on how to handle codacy issues in the future. |
|
I have revised the codacy failure criteria to only fail for major issues. So you can ignore anything that isn't major in the classes you have added (but it would be good to clean up any easily resolved issues). |
|
@cnicol-gwlogic @swfwmd what is the status of this PR? Should it be closed or is it still a work in progress? |
|
@jdhughes-usgs - this can be closed, covered in #1261. Thanks. |
|
changes in PR #1261 |
Created new folder modflowusg.
Created mfusg.py file for ModflowUsg class
Created mfusgbcf.py file for ModflowUsgBcf class
Created mfusgcln.py file for ModflowUsgCln class
Created mfusggnc.py file for ModflowUsgGnc class
Created mfusglpf.py file for ModflowUsgLpf class
Created mfusgsms.py file for ModflowUsgSms class
Created mfusgwel.py file for ModflowUsgWel class