Skip to content
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

fix(binary): fix binary header information #1848

Closed
wants to merge 3 commits into from

Conversation

jdhughes-usgs
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #1848 (4ef943f) into develop (edb70b4) will increase coverage by 0.0%.
The diff coverage is 85.7%.

@@           Coverage Diff           @@
##           develop   #1848   +/-   ##
=======================================
  Coverage     72.3%   72.3%           
=======================================
  Files          255     255           
  Lines        56108   56122   +14     
=======================================
+ Hits         40573   40591   +18     
+ Misses       15535   15531    -4     
Impacted Files Coverage Δ
flopy/utils/binaryfile.py 80.0% <ø> (ø)
flopy/utils/datafile.py 74.0% <ø> (ø)
flopy/mf6/data/mffileaccess.py 72.3% <85.7%> (+0.5%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

@spaulins-usgs spaulins-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. My only concern is that you are changing some of the header variable names in datafile.py (ncol/nrow/ilay to m1/m2/m3) and that if a user was directly creating a binary header this change could break their code. However, I do not believe that users are creating their own MF6 binary file headers using flopy, so it might not be necessary to warn users of this interface change.

@jdhughes-usgs
Copy link
Contributor Author

Looks good. My only concern is that you are changing some of the header variable names in datafile.py (ncol/nrow/ilay to m1/m2/m3) and that if a user was directly creating a binary header this change could break their code. However, I do not believe that users are creating their own MF6 binary file headers using flopy, so it might not be necessary to warn users of this interface change.

I agree this could create problems for people who might be using this directly. Hopefully know one is since the vardis, vardisv and vardisu methods are specific to MODFLOW 6 and really was not consistent with the mf6io document.

jdhughes-usgs added a commit to jdhughes-usgs/flopy that referenced this pull request Jul 16, 2023
@jdhughes-usgs jdhughes-usgs deleted the binary-mod branch July 18, 2023 15:07
wpbonelli pushed a commit that referenced this pull request Aug 25, 2023
* modify pip upgrade in rtd workflow

Replaces PR #1848 (Close #1848)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants