Comprehensive support for MODFLOW-USG ("USG Transport" version) #2575
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2575 +/- ##
===========================================
+ Coverage 55.5% 72.5% +16.9%
===========================================
Files 644 667 +23
Lines 124135 128601 +4466
===========================================
+ Hits 68947 93239 +24292
+ Misses 55188 35362 -19826
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Looks like you put some work into this, @aymanalz. Would be great to get this in. I see there are still some lint and test errors that could use some tending. And I guess there is the general question about the approach for this PR. It seems like some of the classes (BCF, for example), are duplicates of the MF2005 versions with the additional changes for MODFLOW-USG. While this is not ideal, the changes between MFUSG BCF and MF2005 BCF are substantial and I wonder about the best path forward. I think @cnicol-gwlogic has done a nice job trying to minimize code duplication in support of MFUSG so far. With MFUSG having a full-featured life of its own, perhaps it's best to accept the duplicated code and let the MFUSG versions move forward? @cnicol-gwlogic, any thoughts on this?
|
Thanks, Chris, for your comments. I addressed the linting issues. All tests passed now. I agree with you that there is redundancy in the code. I plan resolve these gradually over the following weeks. |
|
It is a lot of work, thank you. I've only had a quick look, but one thought that comes to mind on @christianlangevin 's BCF point is that we could try to make a bunch of the load routines common to lpf and BCF (move them to an external class or we could even have a base package that deals with common BCF and lpf stuff, and then we override that for lpf and bcf). Whilst there are differences between the two packages they are not as large as those between mf2005 BCF and usg's BCF. Although it would be ideal, I think abstracting down usg-transport flopy stuff would be hard because each package can be so different in structure, and there are so many evolving options. Lots of tests here so I think we should be pretty well covered as it stands. |
There was a problem hiding this comment.
@aymanalz, I think this is looking really good. For the future, we might consider whether or not we should include all of USG transport examples in the flopy repository, but since they seem to be relatively small, I think this is okay for now, and seems critical for the current testing approach. You might take one last look through the examples and make sure there are no extraneous files. I see there is also a docx file in there, which we probably don't want to version. When you are satisfied, let us know and we will merge.
There was a problem hiding this comment.
Should probably remove from version control
flopy/mfusg/data.py
Outdated
There was a problem hiding this comment.
Can this be removed? data.py is an empty python file
There was a problem hiding this comment.
Thank you, Josh! The file has been removed.
christianlangevin
changed the title
christianlangevin
changed the title
christianlangevin
changed the title
|
Thanks for this @aymanalz! |
4 participants
This PR is related to a previous one where several changes were made to mfusg. The PR changes were not merged due to test errors. I worked on resolving these errors and merged with the most up-to-date "develop" branch. Tests were run locally without issues.