feat(pathlike): support pathlike throughout public interface #1730

wpbonelli · 2023-02-23T14:16:57Z

Working toward #1729, continuation of #1712, #1603, and #1583

update code where needed to support os.Pathlike in user-facing API
update docstrings and type hints
update most tests to use Path instead of str
add tests checking both Path and str for core classes/utilities
addverbose flag to more utility methods to optionally enable/suppress output
update some notebooks to use pathlib, but not all
add sim_path property to MFSimulation as a shortcut to simulation_data.mfpath.get_sim_path()
- guaranteed Path whether user provides workspace as str or PathLike
- update tests and mf6 tutorial notebook to exercise/demonstrate
preserve str properties where this was already the convention
- e.g. BaseModel.model_ws
- could consider refactoring to Path in future, but may break usages
update resolve_exe function, add tests, and use it from both BaseModel as well as run_model()

codecov · 2023-02-23T14:37:02Z

Codecov Report

Merging #1730 (9ac6f66) into develop (cf3a517) will increase coverage by 0.1%.
The diff coverage is 82.0%.

@@            Coverage Diff            @@
##           develop   #1730     +/-   ##
=========================================
+ Coverage     71.9%   72.1%   +0.1%     
=========================================
  Files          253     253             
  Lines        56004   56043     +39     
=========================================
+ Hits         40282   40416    +134     
+ Misses       15722   15627     -95

Impacted Files	Coverage Δ
flopy/plot/map.py	`82.3% <ø> (ø)`
flopy/utils/formattedfile.py	`88.8% <ø> (-0.2%)`	⬇️
flopy/utils/compare.py	`40.7% <42.8%> (+10.3%)`	⬆️
flopy/utils/zonbud.py	`66.7% <66.6%> (-0.1%)`	⬇️
flopy/export/utils.py	`65.3% <72.3%> (+0.1%)`	⬆️
flopy/mbase.py	`69.7% <81.9%> (+0.4%)`	⬆️
flopy/utils/datafile.py	`74.0% <83.3%> (+0.1%)`	⬆️
flopy/utils/modpathfile.py	`86.0% <83.3%> (+<0.1%)`	⬆️
flopy/utils/gridgen.py	`84.4% <88.8%> (+0.1%)`	⬆️
flopy/export/shapefile_utils.py	`90.0% <90.0%> (-0.3%)`	⬇️
... and 29 more

mwtoews

Thanks, looks good so far, see following comments and suggestions. Are there a few representative tests that use str remaining?

flopy/export/utils.py

flopy/mbase.py

flopy/utils/zonbud.py

flopy/export/netcdf.py

wpbonelli · 2023-02-24T03:53:35Z

Added/expanded a few tests covering both str and Path for Modflow and MFSimulation read/write, gridgen, some export utilities. Planning to add a few more.

mwtoews

Nice stuff, getting closer, see attached suggestions/comments. I'm primarily looking at the main module, and have not seen many of the edits to notebooks or autotests.

flopy/utils/compare.py

flopy/mbase.py

flopy/mf6/mfbase.py

langevin-usgs

Looks great @w-bonelli. Nice addition. As you mentioned, a few remaining os.path.join remain in notebooks, but we can get those later.

langevin-usgs · 2023-03-02T00:42:31Z

flopy/export/netcdf.py

+            output_filename = str(
+                Path(output_filename).expanduser().absolute()


Out of curiosity, why is str needed here?

it's not strictly — previously output_filename was a str, switching to Path would require changes to L160 and L523 and I figured it may be safer to keep it a string in case any external usages had similar expectations

wpbonelli · 2023-03-02T13:06:54Z

@mwtoews thanks for the catches, I think they've all been fixed.

mwtoews

Just a few (probably) final comments. And again, I'm only reviewing the changes in flopy/*

flopy/mbase.py

mwtoews · 2023-03-02T20:39:59Z

flopy/mbase.py

-    model_ws="./",
-    scrub=False,
+    exe_name: Union[str, os.PathLike],
+    namefile: Optional[str],


Currently this is a required parameter, because it does not have a default e.g. = None.

The docstring indicated namefile=None could be used for models that don't require namefiles — the function seems to handle this case OK, just omits it from the args passed to the executable

flopy/export/shapefile_utils.py

- support pathlike or str paths in public APIs - add verbose flag to more utility functions - add sim_path property to MFSimulation - update some notebooks to use pathlib - update most autotests to use pathlib - add unit tests for path arguments - update docstrings and type hints

wpbonelli · 2023-03-03T16:52:03Z

Fixed more docstrings/typehints, thanks @mwtoews, also updated exe resolve function introduced in #1727

add docstrings
test cases described in bug: location of mf6 executable using relative path #1633
log externally (factor silent and model_ws params out)
name resolve_exe_name -> resolve_exe (full path is returned if successful)

mwtoews

Thanks, looks good! I only have a minor suggestion remaining. This PR could be merged soon.

autotest/test_grid.py

wpbonelli force-pushed the support-pathlike branch from 4118618 to b9023dc Compare February 23, 2023 14:54

mwtoews reviewed Feb 23, 2023

View reviewed changes

flopy/export/utils.py Outdated Show resolved Hide resolved

flopy/mbase.py Outdated Show resolved Hide resolved

flopy/mbase.py Outdated Show resolved Hide resolved

flopy/utils/zonbud.py Outdated Show resolved Hide resolved

flopy/export/netcdf.py Outdated Show resolved Hide resolved

wpbonelli force-pushed the support-pathlike branch 2 times, most recently from 38f4499 to 10cc5a0 Compare February 24, 2023 03:50

wpbonelli force-pushed the support-pathlike branch 3 times, most recently from f09aba2 to 1b4102d Compare February 27, 2023 16:07

wpbonelli marked this pull request as ready for review February 27, 2023 17:39

wpbonelli force-pushed the support-pathlike branch 6 times, most recently from c5e98f3 to d0b83c6 Compare February 28, 2023 16:22

wpbonelli requested review from mwtoews, aleaf and langevin-usgs February 28, 2023 17:02

mwtoews reviewed Mar 1, 2023

View reviewed changes

langevin-usgs approved these changes Mar 2, 2023

View reviewed changes

wpbonelli force-pushed the support-pathlike branch from 97cdea4 to 2b241c7 Compare March 2, 2023 05:10

aleaf mentioned this pull request Mar 2, 2023

Feat pyproj crs #1737

Merged

wpbonelli force-pushed the support-pathlike branch 2 times, most recently from 3db3072 to 008f1c5 Compare March 2, 2023 17:19

mwtoews reviewed Mar 2, 2023

View reviewed changes

wpbonelli added 3 commits March 2, 2023 21:28

fix(pathlike): fix docstrings and impl, add test for compare fns

66c7fd8

test(pathlike): add tests and update docstrings for run_model()

adaa711

wpbonelli force-pushed the support-pathlike branch from dfc6829 to 99903b6 Compare March 3, 2023 02:53

refactor: update resolve_exe() fn, various pathlib support fixes

628c937

wpbonelli force-pushed the support-pathlike branch from 99903b6 to 628c937 Compare March 3, 2023 14:35

wpbonelli added 4 commits March 3, 2023 10:13

test(resolve_exe): expand test for resolve_exe function

8dd23d0

test(resolve_exe): separate test cases

593ac2a

test(resolve_exe): cleaner parametrization

a3dbc87

fix run_model tests

6ec7c76

test(run_model): test with rel exe path (repro modflowpy#1633)

b32247b

mwtoews approved these changes Mar 3, 2023

View reviewed changes

autotest/test_grid.py Outdated Show resolved Hide resolved

refactor(test_grid): address PR comments

9ac6f66

wpbonelli merged commit 656751a into modflowpy:develop Mar 3, 2023

wpbonelli deleted the support-pathlike branch March 3, 2023 20:22

wpbonelli mentioned this pull request Mar 6, 2023

bug: location of mf6 executable using relative path #1633

Closed

wpbonelli mentioned this pull request May 21, 2023

feature: broader path-like support #1729

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pathlike): support pathlike throughout public interface #1730

feat(pathlike): support pathlike throughout public interface #1730

wpbonelli commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

mwtoews left a comment

wpbonelli commented Feb 24, 2023

mwtoews left a comment

langevin-usgs left a comment

langevin-usgs Mar 2, 2023

wpbonelli Mar 2, 2023

wpbonelli commented Mar 2, 2023

mwtoews left a comment

mwtoews Mar 2, 2023

wpbonelli Mar 3, 2023

wpbonelli commented Mar 3, 2023

mwtoews left a comment

		output_filename = str(
		Path(output_filename).expanduser().absolute()

feat(pathlike): support pathlike throughout public interface #1730

feat(pathlike): support pathlike throughout public interface #1730

Conversation

wpbonelli commented Feb 23, 2023 • edited Loading

codecov bot commented Feb 23, 2023 • edited Loading

Codecov Report

mwtoews left a comment

Choose a reason for hiding this comment

wpbonelli commented Feb 24, 2023

mwtoews left a comment

Choose a reason for hiding this comment

langevin-usgs left a comment

Choose a reason for hiding this comment

langevin-usgs Mar 2, 2023

Choose a reason for hiding this comment

wpbonelli Mar 2, 2023

Choose a reason for hiding this comment

wpbonelli commented Mar 2, 2023

mwtoews left a comment

Choose a reason for hiding this comment

mwtoews Mar 2, 2023

Choose a reason for hiding this comment

wpbonelli Mar 3, 2023

Choose a reason for hiding this comment

wpbonelli commented Mar 3, 2023

mwtoews left a comment

Choose a reason for hiding this comment

wpbonelli commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading