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(HeadUFile): repro & fix #1503, add tests #1510

Merged
merged 1 commit into from Aug 26, 2022

Conversation

wpbonelli
Copy link
Contributor

@wpbonelli wpbonelli commented Aug 23, 2022

This PR includes:

  • add get_lni(ncpl, *nodes) function in flopy.utils.gridutil.py (convert node number to within-layer node index)
  • use get_lni(ncpl, *nodes) in HeadUFile.get_ts(idx) (fix bug: error when call get_ts() function of class HeadUFile #1503)
  • add get_lni(*nodes) method to Grid
  • declare nlay property on Grid
  • add addopts -ra to pytest.ini (show more info about skipped and xfailed tests)
  • add pytest-cases to testing dependencies in setup.cfg and etc/environment.yml
  • use pytest-cases to define grids in test_grid_cases.py and parametrize tests in test_grid.py
  • run linter on example scripts, tutorials and tests
  • mark EPSG-code related tests flaky

HeadUFile.get_ts() fix

As reported in #1503 HeadUFile.get_ts() was converting node number to within-layer node index incorrectly, causing an IndexError. This wasn't revealed before because it only happened for nodes in layers 2+ and tests only called get_ts() on nodes in layer 1. Now the index conversion is done in function get_lni(ncpl, *nodes) (for "layer-node-index") in new module flopy.utils.gridutil.py. The function's parameters are:

  • ncpl: single int or collection of int if node count varies per layer
  • *nodes zero or more node numbers

The return value is a tuple (layer, node index) if a single node number was provided, otherwise a list of tuples. If no node numbers are provided, the values for all nodes will be returned in ascending order by node number.

HeadUFile.get_ts(idx) is now tested for every node of a sample grid

Grid updates

Adds corresponding method get_lni(*nodes) to Grid, which uses the standalone function above internally.

Also adds unimplemented nlay property to Grid, following convention for other props shared by all grid types.

Test fixtures & parametrization

Per @Hofer-Julian's suggestion I tried out pytest-cases on some tests in test_grid.py. This plugin provides mechanisms to extend or work around limitations of pytest fixtures (e.g. the inability to use them to parametrize a test function).

A number of tests in test_grid.py previously built and tested their own custom grids. Some of these have been pulled into the GridCases class in test_grid_cases.py, which now holds a few examples of each grid type. Some tests in test_grid.py are now parametrized with some or all GridCases, depending whether properties under test are shared or specific to a grid type — pytest-cases lets you subset via prefix, e.g. naming structured grids structured_* and selecting with e.g. @parametrize_with_cases("grid", cases=GridCases, prefix="structured").

The grids are still defined by hand. A future option might be generating grids from a random seed and some size/complexity parameters. pytest-cases supports dynamically generating cases, if pytest_generate_tests is not sufficient. Maybe something like hypothesis could be useful, assuming it's compatible with pytest.

@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #1510 (c730428) into develop (58938b9) will decrease coverage by 0.0%.
The diff coverage is 85.7%.

❗ Current head c730428 differs from pull request most recent head 1d890bf. Consider uploading reports for the commit 1d890bf to get more accurate results

@@            Coverage Diff            @@
##           develop   #1510     +/-   ##
=========================================
- Coverage     72.4%   72.4%   -0.1%     
=========================================
  Files          250     251      +1     
  Lines        54039   54055     +16     
=========================================
+ Hits         39174   39183      +9     
- Misses       14865   14872      +7     
Impacted Files Coverage Δ
flopy/discretization/unstructuredgrid.py 81.7% <60.0%> (-0.4%) ⬇️
flopy/discretization/grid.py 74.5% <66.6%> (-0.2%) ⬇️
flopy/utils/binaryfile.py 79.2% <91.6%> (+0.2%) ⬆️
flopy/utils/grid.py 100.0% <100.0%> (ø)
flopy/plot/crosssection.py 62.4% <0.0%> (-1.6%) ⬇️
flopy/utils/get_modflow.py 56.2% <0.0%> (+0.7%) ⬆️
flopy/utils/mfreadnam.py 78.7% <0.0%> (+1.5%) ⬆️

@Hofer-Julian
Copy link
Contributor

Hypothesis is compatible with pytest. We use it successfully in multiple projects.

* add flopy.utils.grid.py and get_lni(ncpl, *nodes) function ('layer node index')
* add get_lni(*nodes) method to grids
* add tests for new functions & methods
* use get_lni in HeadUFile.get_ts(idx) (fix modflowpy#1503)
* test HeadUFile.get_ts(idx) for every grid node
* run linter on examples and tests
* add addopts = -ra to pytest.ini (show info for skipped/xfailed tests)
* add pytest-cases to setup.cfg and environment.yml test dependencies
* use pytest-cases to define grids & parametrize grid tests
* mark EPSG test cases flaky
@langevin-usgs
Copy link
Contributor

@w-bonelli go ahead and merge this in.

@jdhughes-usgs jdhughes-usgs merged commit 8f13420 into modflowpy:develop Aug 26, 2022
@wpbonelli wpbonelli deleted the tests branch August 29, 2022 01:34
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* add flopy.utils.grid.py and get_lni(ncpl, *nodes) function ('layer node index')
* add get_lni(*nodes) method to grids
* add tests for new functions & methods
* use get_lni in HeadUFile.get_ts(idx) (fix modflowpy#1503)
* test HeadUFile.get_ts(idx) for every grid node
* run linter on examples and tests
* add addopts = -ra to pytest.ini (show info for skipped/xfailed tests)
* add pytest-cases to setup.cfg and environment.yml test dependencies
* use pytest-cases to define grids & parametrize grid tests
* mark EPSG test cases flaky
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* add flopy.utils.grid.py and get_lni(ncpl, *nodes) function ('layer node index')
* add get_lni(*nodes) method to grids
* add tests for new functions & methods
* use get_lni in HeadUFile.get_ts(idx) (fix modflowpy#1503)
* test HeadUFile.get_ts(idx) for every grid node
* run linter on examples and tests
* add addopts = -ra to pytest.ini (show info for skipped/xfailed tests)
* add pytest-cases to setup.cfg and environment.yml test dependencies
* use pytest-cases to define grids & parametrize grid tests
* mark EPSG test cases flaky
wpbonelli added a commit that referenced this pull request Dec 14, 2022
* add flopy.utils.grid.py and get_lni(ncpl, *nodes) function ('layer node index')
* add get_lni(*nodes) method to grids
* add tests for new functions & methods
* use get_lni in HeadUFile.get_ts(idx) (fix #1503)
* test HeadUFile.get_ts(idx) for every grid node
* run linter on examples and tests
* add addopts = -ra to pytest.ini (show info for skipped/xfailed tests)
* add pytest-cases to setup.cfg and environment.yml test dependencies
* use pytest-cases to define grids & parametrize grid tests
* mark EPSG test cases flaky
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* add flopy.utils.grid.py and get_lni(ncpl, *nodes) function ('layer node index')
* add get_lni(*nodes) method to grids
* add tests for new functions & methods
* use get_lni in HeadUFile.get_ts(idx) (fix modflowpy#1503)
* test HeadUFile.get_ts(idx) for every grid node
* run linter on examples and tests
* add addopts = -ra to pytest.ini (show info for skipped/xfailed tests)
* add pytest-cases to setup.cfg and environment.yml test dependencies
* use pytest-cases to define grids & parametrize grid tests
* mark EPSG test cases flaky
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.

bug: error when call get_ts() function of class HeadUFile
5 participants