refactor(datafiles): use 0-based kstp/kper indexing

[wpbonelli]

Prompted by #1338

Plan originally proposed/implemented but abandoned (see discussion below):

• use 0-based indexing for both kstpkper attribute and get_kstpkper() method
• deprecate get_kstpkper() method (redundant)
• applies to
  • LayerFile and children
  • BinaryLayerFile and children
  • CellBudgetFile

[codecov]

Codecov Report

Merging #2000 (17f2270) into develop (af8954b) will increase coverage by 0.1%.
Report is 10 commits behind head on develop.
The diff coverage is 78.5%.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #2000     +/-   ##
=========================================
+ Coverage     72.7%   72.8%   +0.1%     
=========================================
  Files          258     259      +1     
  Lines        58828   59101    +273     
=========================================
+ Hits         42797   43058    +261     
- Misses       16031   16043     +12     

Files	Coverage Δ
flopy/utils/datafile.py	73.9% <100.0%> (-0.4%)	⬇️
flopy/utils/formattedfile.py	88.7% <100.0%> (-0.2%)	⬇️
flopy/utils/binaryfile.py	83.1% <75.0%> (+0.1%)	⬆️

... and 22 files with indirect coverage changes

[langevin-usgs]

My recommendation on this would be to deprecate the .kstpkper' member variable, since it doesn't behave the way we want anyway (1-based), and keep the .get_kstpkper()method instead. Maybe we put a leading underscore in front of._kstpkper`? In the modflow6 repo we use the get_kstpkper() method pretty widely.

[wpbonelli]

Ok will leave get_kstpkper() in place. Looking at the base LayerFile the pattern looks like public properties — maybe just a change of kstpkper to 0-based for consistency (or is it better to try to avoid breaking existing usage) and not deprecate or underscore/private? I don't think it's bad to have both, just thought to do opportunistic cleanup.

[wpbonelli]

maybe it would be safest just to clearly document that kstpkper is 1-based and leave it that way

[langevin-usgs]

maybe it would be safest just to clearly document that kstpkper is 1-based and leave it that way

I think it's preferable to leave .kstpkper as one-based in case someone is using it now and relying on that behavior. We might also consider deprecating it, as a way to continue removing remnant one-based information. If we deprecate/remove it, those using it will have to switch to the zero-based get_kstpkper() method.

[jdhughes-usgs]

I am ok with this change although it may cause people who were using .kstpkper directly instead of using get_kstpkper().

[jdhughes-usgs]

maybe it would be safest just to clearly document that kstpkper is 1-based and leave it that way

I think it's preferable to leave .kstpkper as one-based in case someone is using it now and relying on that behavior. We might also consider deprecating it, as a way to continue removing remnant one-based information. If we deprecate/remove it, those using it will have to switch to the zero-based get_kstpkper() method.

We could convert the internal variable to ._kstpkper and make .kstpkper a property that returns one-based values and issue a deprecation warning that .kstpkper will be removed.

[wpbonelli]

Pending more discussion and a decision on whether to deprecate I restored kstpkper to 1-based. This PR now just clarifies 0 vs 1-based indexing in docstrings and some other minor cleanup, no API changes or deprecations