fix(CellBudgetFile): detect compact fmt by negative nlay

[wpbonelli]

• close bug: Reading flows from binary budget file for steady stress period of zero length #1939
  • previously, omitted totim=0 if compact
• detect compact format by negative nlay
• add mf6 test case courtesy of @mbakker7
• test mf2005 with compact/non-compact

[codecov]

Codecov Report

Merging #1966 (b5d6e29) into develop (941a5f1) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           develop   #1966   +/-   ##
=======================================
  Coverage     72.7%   72.7%           
=======================================
  Files          257     257           
  Lines        57800   57802    +2     
=======================================
+ Hits         42023   42044   +21     
+ Misses       15777   15758   -19     

Files	Coverage Δ
flopy/utils/binaryfile.py	82.9% <100.0%> (+1.7%)	⬆️

... and 1 file with indirect coverage changes

[wpbonelli]

Updated CellBudgetFile to distinguish explicitly between compact and non-compact (old-style) files by negative nlay values, as that seems to be the deciding factor here. It is possible I'm misinterpreting.

My understanding is that the _totim_from_kstpkper() method is intended for old-style non-compact files, which may or may not have time data. Previously this method was called if totim==0 — it seems time 0 was meant to implicitly signal that a file is non-compact. This caused _totim_from_kstpkper to be used for compact files containing legitimate records with totim=0, which produced incorrect totim values.

[langevin-usgs]

Hey @wpbonelli, the negative nlay check looks to be more definitive (though I did not verify in the mf2005 source code itself). The one thing that still bugs me in all this is that _totim_from_kstpkper should give a correct totim, even for this particular case where the first stress period has perlen set to zero -- at least that is what it appears based a quick look at that routine.

[wpbonelli]

@langevin-usgs I can come back and add some tests for _totim_from_kstpkper() in a separate PR, if that sounds OK.

[mbakker7]

For what it is worth, the current PR doesn't fix the problem yet of t=0 not being read from the budgetfile in MF6. Was it supposed to be?

[wpbonelli]

@mbakker7 maybe I am missing something, I thought it does fix the t=0 issue. With the provided test model, cbb.get_data(totim=0) now returns a 4-element list, one item for each of STO-SS, FLOW-JA-FACE, WEL, and CHD, identical to what is returned by cbb.get_data(kstpkper=(0, 0))

[array([[[0., 0., 0., 0., 0., 0., 0., 0., 0., 0.]]]),
 array([[[ 0.,  0.,  0., -0.,  0.,  0., -0.,  0.,  0., -0.,  0.,  0.,
         -0.,  0.,  0., -0.,  0.,  0., -0.,  0.,  0., -0.,  0.,  0.,
         -0.,  0.,  0., -0.]]]),
 rec.array([],
          dtype=[('node', '<i4'), ('node2', '<i4'), ('q', '<f8')]),
rec.array([(10, 1, 0.)],
          dtype=[('node', '<i4'), ('node2', '<i4'), ('q', '<f8')])]

The test failures here should resolve after MODFLOW-ORG/modflow6#1370 is merged into mf6 Test failures are resolved.

[mbakker7]

I don't quite understand how the test is passing on your machine. It doesn't really on my machine. Are you sure it is passing? Or did you also change something in MODFLOW6 and I should get a new version of MODFLOW6?

---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[4], line 1
----> 1 test_read_mf6_2sp(sim)

Cell In[3], line 15, in test_read_mf6_2sp(mf6_gwf_2sp_st_tr)
     13 exp_times = [float(t) for t in range(11)]
     14 assert hds.get_times() == exp_times
---> 15 assert cbb.get_times() == exp_times
     17 # check stress periods and time steps
     18 exp_kstpkper = [(0, 0)] + [(i, 1) for i in range(10)]

AssertionError: 

[wpbonelli]

Just checked with mf6.4.1, 6.4.2 and latest nightly build, and all pass.. Could be a python package version issue, would you mind running pip/conda list and sharing output?

[mbakker7]

You really think it could be a python package version issue? That would be surpising. You really want pip list output? It is a long list.

[wpbonelli]

I imagine numpy is the only plausible candidate, maybe just numpy version?