fix(modflow/mflpf) mfusg unstructured lpf ikcflag addition

[cnicol-gwlogic]

Add writing of IKCFLAG to mfusg unstructured models' LPF package.
This was not previously written, which resulted in all flavours of mfusg
throwing an error on LPF load for unstructured models, if any keywords were
specified. No other model types are affected by this change.
Add test to existing t506_test.test_mfusg.

[codecov]

Codecov Report

Merging #1044 (d5cde32) into develop (7dc4326) will increase coverage by 0.009%.
The diff coverage is 56.000%.

@@              Coverage Diff              @@
##           develop     #1044       +/-   ##
=============================================
+ Coverage   71.758%   71.767%   +0.009%     
=============================================
  Files          225       225               
  Lines        51884     51897       +13     
=============================================
+ Hits         37231     37245       +14     
+ Misses       14653     14652        -1     

Impacted Files	Coverage Δ
flopy/mf6/data/mfdatastorage.py	72.937% <0.000%> (-0.408%)	⬇️
flopy/modflow/mflpf.py	94.052% <100.000%> (+2.830%)	⬆️

[christianlangevin]

Hey @cnicol-gwlogic, thanks for this. I see that you've added IKCFLAG as a new argument to LPF and it is written for an unstructured mfusg model. I have a couple of quick questions. (1) By adding IKCFLAG to the write, do you think we should have a corresponding read in the load method? (2) It does not seem to me that LPF is very robust for handling all flavors of input for mfusg. For example, if IKCFLAG is not zero, then Ksat is required, which is of size NJA, but LPF is not setup to handle that. So I guess I'm just wondering about the benefit of adding IKCFLAG if the LPF class in flopy is not really setup yet to work with mfusg. Or maybe this is something you're working on?

One other weird thing I noticed due to syntax highlighting in Pycharm is that nocvcorrection is not passed to the LPF constructor for the load method. Since you are in there now, wondering if you see anything obvious for why that might be?

[cnicol-gwlogic]

Hi @langevin-usgs - no worries.

On point 1) I will add the load method.

On 2) good point. Only reason I did this was because I often use novfc and sometimes constantcv keywords, and when I did, usg was failing because of that missing ikcflag (got keyword text, not a newline (0) or an integer). So I don't want anything other than ikcflag = 0 there. Maybe I can just hard code a 0 for it with no others allowed, removing the pointlessness of allowing ikcflag of 1 and -1 (?).

3. I will have a look at that.

I will resubmit if you are happy with 2) in particular.

[christianlangevin]

Regarding (2) it makes sense to me to just hard code a local variable called ikcflag to zero for for the condition (self.parent.version == "mfusg" and self.parent.structured == False). If hard coded, then seems like you can remove the docstrings and ikcflag argument. Would be good at some point to get this all wired up correctly for mfusg. Not sure if we should make a new flopy class altogether (lpfu?) for just the unstructured case or try to weave it into lpf. Thanks for the contribution.

[cnicol-gwlogic]

No probs Chris, thanks. My latest commits should sort these out. I added one more commit to revert back my accidental black formatting of t506_test.py - apologies.

The new unstructured lpf class - yeah it could get messy/big pretty quickly, and I think most users will move or have already moved to mf6 (really slowly in my case).

Having said that, thinking through this now, I will shortly need to implement some of the fancier mfusg (beta/transport) lpf options too (richards equation / bubble point). If you are open to allowing those sorts of things come in, I could see use for an unstructured lpf class. But I guess the issue then is do we bother having tests for those fancier options (and do we run the beta models in tests or not). It probably opens a can of worms, and might be best left out of flopy. But I am happy to start something like that if you think it fits.