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

refactor: simplify autotest framework #1464

Merged
merged 28 commits into from
Dec 11, 2023

Conversation

wpbonelli
Copy link
Contributor

@wpbonelli wpbonelli commented Nov 23, 2023

Changes include

  • fold TestSimulation into TestFramework
  • rename/refactor properties
    • simpath -> workspace
    • build_function -> build (and inject in __init__)
    • exfunc -> check
    • exe_dict -> targets
    • require_failure -> xfail
    • action -> compare
    • fold switches run_comparison, make_comparison, mf6_regression into compare (optional str), accepts:
      • "auto" picks comparison type based on workspace contents, equivalent to previous "compare" (default)
      • "run_only" equivalent to previous run_comparison=True and make_comparison=False
      • "mf6_regression" equivalent to previous mf6_regression=True
      • None disables comparison
  • standardize test function names, step toward automatic discovery?
    • build_models creates models
    • check_output checks results
  • framework doesn't need to know about test case indices, remove idxsim and just use lambda if build_models() needs a test case index
  • support pathlib.Path or str paths
  • run black/isort, format test description docstrings
  • remove unused functions from common_regression.py
  • remove pytest-cases from tests and as a test dependency
  • fix hardcoded gridgen left in conftest.py from test: require dev exes, warn/skip if download/rebuilt not found #1460

Merge considerations

Future directions

  • support multiple comparison/regression models? (e.g. compare current mf6 with previous mf6 and mf2005, or PRT with mp6 and mp7)
  • explicit bookkeeping for sim/model workspaces? (replace implicit expectation that test and framework agree)
  • autodiscovery of test cases following e.g. this pattern?

@wpbonelli wpbonelli marked this pull request as ready for review November 24, 2023 20:20
@@ -160,15 +161,15 @@ def setup_comparison(namefile, dst, remove_existing=True):
print("Could not make " + dst)
# clean directory
else:
print(f"cleaning...{dst}")
print("Cleaning ", dst)
Copy link
Contributor

Choose a reason for hiding this comment

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

The change is fine but I am wondering if there is a reason to move away from f-strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use f-strings consistently and wrap paths with single quotes e.g. Cleaning 'some/folder' as it's a bit easier to read

# Possible comparison - the order matters
optcomp = (
"compare",
OPTCOMP = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the change to uppercase for defined data.

Copy link
Contributor Author

@wpbonelli wpbonelli Dec 4, 2023

Choose a reason for hiding this comment

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

I meant to move this to a top-level variable so other modules could import it, will do so if you agree

edit: done and renamed to COMPARE_PATTERNS

autotest/common_regression.py Outdated Show resolved Hide resolved
* fold TestSimulation into TestFramework class
* rename simpath -> workspace
* rename build_func -> build
* rename exfunc -> check
* rename exe_dict -> targets
* support pathlib.Path throughout
* steps toward unifying framework function signature
…avor of enumeration for simpler external API, still kluged internally
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 2, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 2, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 17, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 17, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 17, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 19, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 19, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 19, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 19, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 19, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 19, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 26, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 26, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Jan 26, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 6, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 6, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 6, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 8, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 8, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 8, 2024
wpbonelli pushed a commit to wpbonelli/modflow6 that referenced this pull request Feb 9, 2024
wpbonelli pushed a commit to wpbonelli/modflow6 that referenced this pull request Feb 9, 2024
wpbonelli pushed a commit to wpbonelli/modflow6 that referenced this pull request Feb 9, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 9, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 9, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 9, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 9, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 9, 2024
emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Feb 9, 2024
langevin-usgs pushed a commit that referenced this pull request Feb 9, 2024
* feat(GWE): Introduce Groundwater Energy Transport

* fix typo in meson file

* fix a formating issue that was popping up in an MST autotest

* some legacy line leftover from a botched rebase, possibly

* Need update initial autotest to conform to new autotest standards adopted with #1464

* Start looking for post-rebase breakages

* Get gwfgwe and gwegwe exchanges up-to-date based on #1505

* Code that had been moved to set_active_status in FMI was still present in fmi_fc(). Removing.

* Forgot to remove unused variables after making changes in 4d76729

* Adding another autotest specific to GWE

* Adding another autotest after getting it #1464 compliant

* Compliantizing another new autotest with PR #1464

* Fixes in response to #1493 (comment)

* Fix in response to #1493 (comment)

* Rerunning black in response to #1493 (comment)

* Fix in response to #1493 (comment)

* Made changes to dfn files in response to #1493 (comment) and #1493 (comment) and reran mf6ivar.py, which updated the tex files in this commit

* Fix in response to #1493 (comment)

* remove unnecessary line of script

* Add missing lines to ConnectionBuilder.f90 related to GWE

* Add a GWE vs GWT comparison autotest

* GWE-GWE exchanges now working.  Other clean-up for code uniformity

* Rebrand constant temperature package acronym to CTP

* Missed an import renaming update

* Forgot to remove a now obsolete file due to renaming.

* Rebrand energy storage and transfer package acronym to EST

* Forgot to remove a now obsolete file due to renaming (again)

* Rebrand gwe dispersion package acronym to CND (conduction) since that is the dominant process in heat transport

* Remove gwe-related code from generalized transport code (tsp1.f90 & tsp1ssm1.f90)

* add single-cell test for energy source loading (ESL) package

* Adding energy source loading (ESL) package

* remove unused variable

* fprettify

* Add another ESL autotest

* Adding another autotest that compares gwe to three different analytical solutions from Carslaw & Jaeger (1947)

* Bringing over Stallman autotest from previous GWE PR (#1237)

* Adding streamflow energy transport (SFE) package

* Add autotest for SFE

* Adding lake energy transport (LKE) package. Includes new autotest

* forgot meson update

* Adding multi-aquifer well energy transport (MWE) package. Includes new autotest

* Adding unsaturated-zone energy transport (UZE) package. Includes 2 new autotests

* Attempting to reapply a failing autotest. Unable to discern why it is failing as downloaded contents from its failure are working locally.

* removing troublesome autotest.  Downloaded contents from failed run on Actions doesn't yield the same output that the logs are reporting.  This particular autotest has an analytical solution (and a plot) that might be better shown on the modflow6-examples repo anyway

* Removing a file that shouldn't have been added (snuck in among other staged files)

* Update release notes
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.

3 participants