-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(datafile): ignore "text" parameter, add attributes from file #2231
base: develop
Are you sure you want to change the base?
Conversation
6d2abab
to
2f1d78e
Compare
This does have a breaking change, where previously the Before: >>> pth = example_data_path / "unstructured/headu.githds"
>>> obj = flopy.utils.binaryfile.HeadUFile(pth)
>>> obj.text
b'headu' After: >>> obj = flopy.utils.binaryfile.HeadUFile(pth)
>>> obj.text
'headu'
>>> obj.text_bytes
b' HEADU' if this is a deal-breaker, then Also, I should note that this PR does not change FormattedHeadFile, since further refactoring may be needed. |
2f1d78e
to
fb14357
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2231 +/- ##
=========================================
- Coverage 70.8% 70.8% -0.1%
=========================================
Files 294 294
Lines 59026 59029 +3
=========================================
- Hits 41832 41804 -28
- Misses 17194 17225 +31
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is a deal-breaker, then obj.text can be kept as the modified bytes (possibly raising a deprecation message if accessed), and the str version of the attribute instead stored at obj.text_str.
I think changing .text
to str could be considered a fix rather than a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwtoews, there is one potential issue that I see here, and I'm not sure of the best way to handle it. With older MODFLOW versions (2000/2005/NWT), it's possible to write different types of records to the same binary file (for example both head and drawdown can be written as alternating records in a binary file). I think we included that "text" argument as a way to deal with this, though maybe it wasn't handled properly. The code indicates that we do not store the header entries for text strings that do not match. One option might be to preserve this behavior in HeadFile/UcnFile/HeaduFile and begin user transition to a more generic version of BinaryLayerFile (or better named alternative), which could just work on any variation. We now use HeadFile to read the dependent variable information for the advanced packages in MODFLOW 6, including SFR, LAK, MAW, UZF, and their transport counterparts. It's not ideal to use a class named HeadFile to read these, and it's also not ideal that a user needs to specify the text string, because for MODFLOW 6 only one type of data will be found in these files. For mf6 flavors of "HeadFile" eliminating the effect of the text arg is a welcome addition.
Thanks @langevin-usgs for the info, I'll create and add a mixed text example to test, e.g. head/drawdown. The text parameter can be changed to optional, which would default to read the first text header, and warn if more than one found. |
Confirmed I can create the mixed-text file with older MF versions, and testing current and ancient versions of flopy I can see this feature has never worked. I'll superseded this PR with a fix after a few other related refactors that use the |
This PR ignores the
text=
parameter fromHeadFile
,HeadUFile
andUcnFile
, since this should be read from the file and is constant for all records. This PR adds these attributes:obj.text_bytes
, the 16-byte char bytes obtained from the fileobj.text
, astr
version of this field, .e.g., used byflopy.export
for NetCDF variable namesThere are instances where other binary files can be attempted to be read (e.g. "mt3d_test/mf96mt3d/P01/case1b/MT3D001.UCN" with HeadFile), but these are now caught with exceptions raised. Warnings are raised if the text records are inconsistent, which shouldn't happen (unless I've misunderstood something!)