Refactor and clean up dapper, and some bug fixes by rfiorella · Pull Request #50 · NGEE-Arctic/dapper

rfiorella · 2026-09-30T16:59:52Z

This PR removes some of the duplication left over from some LLM code generation steps without changing behavior. It also adds characterization tests, applies a set of approved breaking changes, and fixes seven bugs. Each concern is its own commit, so no commit mixes a refactor, a breaking change and a bug fix.

Summary

Metric	Before (`8b43332`)	After
Package lines (both ruff-formatted)	15,049	13,665 (−9%)
ruff errors (project config)	412	1
mypy errors	100	88
Coverage	36%	49%
Tests	47 passed / 1 skipped	82 passed / 1 skipped

The six original test files are byte-identical to the baseline. The only change under tests/ is five new files, test_characterize_*.py, all approved in advance.

Refactor (no behavior change)

Tooling and style: added ruff and mypy config and ran ruff format. The formatted code is AST-identical to the original, and the formatting commit is listed in .git-blame-ignore-revs so it doesn't clutter blame. Also tidied imports and modernized type annotations.
Dead code removed: unreachable code in cmip_utils, the exporter's raw path (never called), unused private helpers, and guards that could never trigger.
Docstrings and comments: fixed stale or wrong docstrings and replaced 27 placeholder module docstrings.
Deduplication: the same logic had been written several times with small differences, so a fix in one copy didn't reach the others. Now there is one copy of each:
- one shared attribute-merge helper instead of three; this also replaces the deprecated datetime.utcnow
- a single Feb-29 helper
- a shared _numeric_dtime
- a single LonWrap
- normalize_lons
Exporter and Domain tidy-ups: a shared provenance-column constant replaces repeated string lists, and redundant conversions are removed.
Checked against the old code: where logic was rewritten, old and new implementations give identical results (create_dtime over 292 cases, chunking over 225 cases, and elm_data_dicts).

Potentially breaking changes

Unused public helpers removed from integrations.earthengine.gee_utils, elm.utils, io.attrs/fs/provenance, surf.schema, met.cmip_utils, surf.sfile and met.validation. None of them is referenced in src, tests, docs or tutorials. The full list is in commit 0ec1528.
Domain.make_topounits now raises RuntimeError when a topounit build fails. It is annotated -> Domain, but it used to catch the RuntimeError, print it and return None. A call like dom = dom.make_topounits(...) therefore replaced the Domain with None without any error. This is also a bug fix.
Importing dapper.met.validation no longer calls matplotlib.use("Agg"). Forcing that backend at import time broke inline and interactive plotting in notebooks. It wasn't needed, because quicklooks only save PNGs and close their figures.
dapper.__version__ now comes from package metadata, so it reads 1.0 (the version in pyproject.toml) instead of the hardcoded 0.1.0.
Dependencies now match actual imports:
- Added shapely, pyproj, fsspec, python-dateutil and intake, which the code imports but were never declared.
- Dropped pip.
- jupyter, jupytext, geemap and rioxarray move to a notebooks extra; folium goes in a new plot extra.
- A fresh install without extras passes the full suite.
Deprecated customize_surface(engine=) and ERA5Adapter.id_column_for_csv. Both still work but now emit a DeprecationWarning. engine was never used and now defaults to None.

Bug fixes (one commit each, each with a regression test)

RH above 100% from compute_humidities (elm/utils.py, 3a3b684): saturation vapor pressure used the latent heat of vaporization for T ≥ 273.15 K. Actual vapor pressure used it for T ≤ 273.15 K, which is the opposite choice. When the dewpoint equalled the air temperature, the two came out different, so RH went above 100%, and RH/QBOT were biased for ERA5 inputs. Both now choose the phase from temperature the same way.
One NaN wrecked MET packing in sites mode (elm/utils.py, 517b0e4): in sites-mode MET export, pack_params gets each site's raw series. It used data.min()/data.max(), so a single NaN made add_offset and scale_factor NaN. The exporter then fell back to scale_factor=1.0, which under int16 packing destroys small-magnitude variables such as QBOT and PRECTmms. The fix uses np.nanmin/np.nanmax, as the cellset global path already did.
ELM domain.nc corners in the wrong order (domains/elm_domain.py, 65bd3b5): cell corners xv/yv were written as [minx, maxx, minx, maxx] / [miny, miny, maxy, maxy]. That is a Z order, not the counter-clockwise order (ll, lr, ur, ul) that CIME/ELM domain files use. As a result, Domain.from_elm_domain read dapper-written domain files back as self-intersecting, zero-area cells. Corners are now written counter-clockwise.
Zonal landuse used the first site's polygon for every site (landuse/landuse.py, 0491500): export_landuse_timeseries called kwargs.setdefault("targets", ...) on a kwargs dict shared across the per-run loop. In sites mode with sampling_method="zonal", the first site's target stuck, so every later site was sampled over the first site's polygon. Each run now gets its own copy of kwargs.
Wrong dimension order in zonal sampling output (geo/zonal.py, 46a3dd4): sample_gridded_dataset_polygons reordered each per-target slice before xr.concat(..., dim=lat_dim), and the concat then put lsmlat first. The output came out as e.g. (lsmlat, time, natpft, lsmlon) instead of ELM's (time, natpft, lsmlat, lsmlon). The reorder now happens after the concat, as sample_gridded_dataset_points already does. This affects zonal landuse and zonal surface outputs.
SurfaceFile.resize_dim failed for any dimension used by a data variable (surf/sfile.py, 3869a9a): it assigned the new-length coordinate before resizing the data variables, so any dimension used by a variable raised "conflicting sizes". Truncation now uses isel. Padding builds the padded variables first and then installs the extended coordinate, keeping dtype, attrs and variable order.
units_policy="warn" never warned, and integer surface variables could not be edited (surf/sfile.py, d66fa7a):
- "warn" behaved exactly like "ignore"; it now emits a UserWarning.
- Overwriting a variable used to force the dtype from the registry. The specs carry no dtype, so that was always the float32 default, which made every integer variable (e.g. URBAN_REGION_ID) uneditable.
- Overwriting an existing variable now keeps its dtype from the file unless a dtype override is given.

jonschwenk · 2026-09-30T22:21:30Z

It's all the stuff I always mean to do but never get around to! Plus extras! For posterity, the initial development of dapper was done without LLMs, then partly with LLMs, and finally fully with LLMs. So it's nice to see a major cleanup effort here to make it more consistent, robust, and hopefully usable.

Pin current behavior of weakly covered code before refactoring: domain export/round trip, landuse sampling, ELM packing/humidity, surface customization, and geo sampling helpers. Assertions marked 'PINS BUG Bn' document known-incorrect behavior logged in REFACTOR_NOTES.md.

ruff: py311 target, line-length 88, rules E4/E7/E9/F/I/UP/B; excludes dev/ scratch scripts and read-only tests. mypy: ignore_missing_imports, excludes dev/. No pytest configuration touched.

Formatting only (AST-identical for all 39 files); dev/ and tests/ excluded.

Sort imports, drop unused and duplicate imports, move module-level imports above the lazy Earth Engine blocks (gee_utils, topomake) and fastparquet helper (exporter), and remove function-local re-imports of modules already imported at module level.

PEP 585/604 syntax (dict/list/tuple, X | None) and unquoted annotations where postponed evaluation is active. Annotations and typing imports only.

- cmip_utils: drop 77 unreachable lines after sample_bbox_means_for_aois returns; unused locals - exporter: drop never-called _pass1_to_parquet_raw and _zone_mappings_path, unused writer params (years_span, filename_template), an unreachable mode check, and redundant pack-scope branches (same results) - zonal: drop unused _reduce_da and per-gid guards that cannot trigger after the global candidate-count check - temporal: drop always-empty accum_vars - sfile: drop unused locals - era5 adapter: drop wind_direction, which was computed then discarded - gee_utils: drop unused _ROOT_DIR/_DATA_DIR - remove commented-out code and an impossible netCDF4 None check

- Exporter: document the real constructor parameters and layouts - ERA5Adapter: describe the actual preprocess steps (Feb 29 kept until temporal alignment, one-hour lookahead) - customize_surface: drop nonexistent validate_units, document units_policy and engine - sample_e5lh/parse_geometry_objects: drop references to nonexistent AOI and Domain.to_geometries - schema: move the module docstring to the top; drop references to nonexistent dapper.surf.write - SurfaceValidator: correct V-105, list V-108/V-109 - replace 27 'dapper module: x.y' placeholder docstrings, drop path header comments, conversational and 'NEW' comments No code changes (verified AST-identical after stripping docstrings).

geo.sampling.write_netcdf, sfile.write_surface_nc, and SurfaceFile.to_netcdf each re-implemented the same attr precedence (append_attrs > source attrs > dapper defaults). Move it to io.attrs.merge_global_attrs. Replace deprecated datetime.utcnow() with io.attrs.utc_timestamp(), which yields the identical string format.

- is_feb29() replaces six hand-written Feb-29 masks (temporal, fluxnet) - create_dtime reuses _numeric_dtime instead of a duplicate copy - one _LINEAR_VARS / _FFILL_VARS definition Verified identical outputs and exceptions against the previous implementation across 292 input combinations.

- one LonWrap definition (geo.lonwrap), re-used by zonal and landuse - zonal and landuse use lonwrap.normalize_lons instead of hand-rolled list comprehensions (same NaN handling) - merge _median_step/_median_step_lon in infer_grid_metadata - drop sfile._latlon_dim_names, unused since the previous cleanup

- share SAMPLING_PROVENANCE_COLUMNS between Domain.to_df_loc and Exporter._site_attrs - read domain.mode directly (domain is always a Domain here) - initialize filename_prefix in __init__ instead of getattr fallbacks - rename misleading import aliases (utils -> fs, dt -> temporal) - simplify zone-mapping lon column selection (same values) - write the tab separator as '\t' instead of a literal tab - type hints on Exporter.__init__ and run

- zip(..., strict=False) to make existing lenient behavior explicit - f-strings instead of str.format; datetime.UTC alias - attribute access instead of constant getattr - immutable default for elm_var_packing_params(data=()) - rename ambiguous loop variables - ruff config: ignore B028 (stacklevel would move warning attribution) and per-file B023 where closures run within the same iteration

from_file re-applied the CRS normalization that from_provided already performs; from_file/from_geometry converted path_out and run_group before from_provided converted them again. Add return types to iter_runs and topounits_for_gid.

_dtype_nbytes' alias table was unreachable (np.dtype accepts every alias it listed) and the per-pattern 'lock axis to 1' branches in _compute_auto_chunks reassigned values that were already 1. Verified identical chunk tuples across 225 combinations.

- collapse duplicated topounits-is-None checks - drop the unused parameter of _parse_spec - use a separate set for skipped columns in add_params_from_df instead of rebinding the list argument - ds.sizes instead of deprecated ds.dims[...] lookup in resize_dim

Required-var lists, the ERA5 raw->ELM name map, and units now come from schemas.elm / config.metsources.era5 where values are identical. Differing values (DTBOT units 'unsure', list-typed ranges, band order) stay literal. Verified identical keys, order, values, and types.

None of these were referenced in src, tests, docs, or tutorials: - integrations.earthengine.gee_utils: split_into_dfs, infer_id_field, featurecollection_to_df_loc - elm.utils: validate_met_vars (needed unshipped docs/data), gen_zone_mappings; elm.utils no longer imports Domain (§5.8) - io.attrs.apply_append_attrs, io.fs.make_directory, io.provenance - surf.schema: register_many, expand_registry, propose_export_policy, EXPORT_POLICIES, validate_against_schema - met.cmip_utils: extract_vars_from_files, cftime_date, summarize_search - surf.sfile: build_surface_dataset, build_surface_dataset_cellset - met.validation: raw-site-parquet/raw-site-csv quicklook modes, whose inputs nothing produces any more BREAKING CHANGE: the names above are no longer importable.

…B13) make_topounits caught RuntimeError, printed it, and returned None despite its '-> Domain' annotation, so 'dom = dom.make_topounits(...)' silently discarded the Domain. The error now propagates. BREAKING CHANGE: Domain.make_topounits raises RuntimeError instead of returning None when a topounit build fails.

…§5.4) Importing dapper.met.validation called matplotlib.use('Agg'), switching the backend for the whole process (e.g. breaking inline/interactive plots in notebooks). Quicklooks only save PNGs and close their figures, so the backend switch is unnecessary; matplotlib already picks Agg when no display is available. Smoke-tested sites and cellset quicklooks. BREAKING CHANGE: importing dapper.met.validation no longer changes the matplotlib backend.

__version__ was hardcoded to 0.1.0 while pyproject declares 1.0. BREAKING CHANGE: dapper.__version__ now reports the installed distribution version (currently '1.0'), or '0+unknown' when not installed.

- add directly imported but undeclared deps: shapely, pyproj, fsspec, python-dateutil, intake - drop pip; move jupyter, jupytext, geemap, rioxarray (not imported by dapper) to a 'notebooks' extra; add a 'plot' extra with folium - sync environment.yml and document the extras Verified the full suite passes on a fresh install without extras. BREAKING CHANGE: jupyter, jupytext, geemap, and rioxarray are no longer installed by default; use 'pip install dapper-elm[notebooks]'.

…column_for_csv (§5.7) Both remain functional but emit DeprecationWarning. engine now defaults to None (it was never used).

In sites-mode MET export, pack_params receives the raw per-site series. data.min()/max() propagated any NaN into add_offset/scale_factor, and the exporter then fell back to scale_factor=1.0, destroying small variables such as QBOT and PRECTmms under int16 packing. Use np.nanmin/np.nanmax (as the cellset global path already does). Updates the T-7 pin for B3 into a regression test.

Saturation vapor pressure used latent heat of vaporization for T >= 273.15 K, but actual vapor pressure used vaporization for T <= 273.15 K (the opposite branches), yielding RH > 100% when the dewpoint equals temperature and biased RH/QBOT for ERA5 inputs. Both now use the temperature-based phase choice. Updates the T-7 pins for B2 into regression tests.

xv/yv were written as [minx,maxx,minx,maxx]/[miny,miny,maxy,maxy], a Z order rather than the counter-clockwise (ll, lr, ur, ul) order used by CIME/ELM domain files. Domain.from_elm_domain therefore read dapper-written domain files back as self-intersecting zero-area cells. Updates the T-1 pins for B4 into regression tests.

export_landuse_timeseries called kwargs.setdefault('targets', ...) on the shared kwargs dict inside the per-run loop, so in sites mode with sampling_method='zonal' every site after the first was sampled over the first site's polygon. Use a per-run copy of kwargs. Updates the T-2 pin for B5 into a regression test.

sample_gridded_dataset_polygons reordered each per-target slice before xr.concat(..., dim=lat_dim), and the concat then prepended lsmlat, producing e.g. (lsmlat, time, natpft, lsmlon) instead of ELM's (time, natpft, lsmlat, lsmlon). Reorder after the concat, as sample_gridded_dataset_points does. Affects zonal landuse and zonal surface outputs. Updates the T-2 pin for B18 into a regression test.

…s (B19) resize_dim assigned the new-length coordinate before resizing data variables, so any dim used by a data variable raised 'conflicting sizes'. Truncation now uses isel; padding builds the padded variables first, then installs the extended coordinate, preserving dtype, attrs, and variable order. Updates the T-8 pin for B19 into a regression test.

…(B15) - units_policy='warn' now emits a UserWarning (it behaved like 'ignore') - overwriting an existing variable keeps its file dtype unless a dtype override is given; registry dtypes are only the float32 default (the specs carry no dtype), which made every integer variable such as URBAN_REGION_ID uneditable Updates the T-8 pins for B15 into regression tests.

rfiorella · 2026-10-16T21:18:50Z

Wonder if we should have a test to see if any of this has changed answers (like a gold file test or something)

jonschwenk · 2026-10-19T20:50:26Z

@rfiorella What is B7? It's apparently unresolved but I didn't see the description anywhere here.

Otherwise I'm glad you put an eye on this because many of the current design decisions were inferred from conversations, example files, and outdated docs.

jonschwenk · 2026-10-19T20:57:06Z

Wonder if we should have a test to see if any of this has changed answers (like a gold file test or something)

Some of the existing tests were developed after bugs had been found, so we wouldn't want to lose those. I wouldn't build a "gold file" test until the repo is more hardened and demonstrated via use--I'm sure more will surface. I guess for a PR this sweeping it might be good to have an LLM do a poor-man's version of it instead of formalizing something? You've already doubled the number of tests and provided much more coverage--I guess the question is if the new tests are correct. We don't know that the old ones were, either.

matthewhoffman

@rfiorella , thanks for this sweeping update. Obviously it is too big to review in any detail, but I did look over all the commit messages individually, and all the changes seem worthwhile. Given the low maturity level of the codebase and little active development, I see no reason not to go ahead and merge this. My only concern would be if anyone has active branches that would be a nightmare to rebase after this gets merged. I do not.

rfiorella · 2026-10-22T03:54:26Z

@rfiorella What is B7? It's apparently unresolved but I didn't see the description anywhere here.

Otherwise I'm glad you put an eye on this because many of the current design decisions were inferred from conversations, example files, and outdated docs.

All of the B* references relate to an intermediate LLM spec markdown file that I used to help guide the refactoring. I excluded the md file in the PR since it's essentially clutter at this point but never went back and updated the PR message. I'll do that and add the bug list this pr claims to have fixed to an additional message here.

rfiorella · 2026-10-26T18:26:31Z

Updated the main PR message to be a bit more descriptive and remove cryptic references to intermediate files that weren't pushed.

@jonschwenk @jsta @matthewhoffman Anyone feel free to merge if you're happy with it now.

rfiorella changed the title ~~Refactor and clean up dapper; approved breaking changes and bug fixes~~ [WIP] Refactor and clean up dapper; approved breaking changes and bug fixes Sep 30, 2026

rfiorella added 28 commits October 14, 2026 09:44

build: add ruff and mypy configuration

7fc8568

ruff: py311 target, line-length 88, rules E4/E7/E9/F/I/UP/B; excludes dev/ scratch scripts and read-only tests. mypy: ignore_missing_imports, excludes dev/. No pytest configuration touched.

style: apply ruff format to src/dapper

996a635

Formatting only (AST-identical for all 39 files); dev/ and tests/ excluded.

chore: ignore formatting commit in git blame

8d33c17

refactor: tidy imports

c722c4d

Sort imports, drop unused and duplicate imports, move module-level imports above the lazy Earth Engine blocks (gee_utils, topomake) and fastparquet helper (exporter), and remove function-local re-imports of modules already imported at module level.

refactor: modernize type annotations

63bb985

PEP 585/604 syntax (dict/list/tuple, X | None) and unquoted annotations where postponed evaluation is active. Annotations and typing imports only.

build!: read dapper.__version__ from package metadata (§5.5)

e16a44f

__version__ was hardcoded to 0.1.0 while pyproject declares 1.0. BREAKING CHANGE: dapper.__version__ now reports the installed distribution version (currently '1.0'), or '0+unknown' when not installed.

feat: deprecate unused customize_surface(engine=) and ERA5Adapter.id_…

0c01d3e

…column_for_csv (§5.7) Both remain functional but emit DeprecationWarning. engine now defaults to None (it was never used).

rfiorella added 2 commits October 14, 2026 09:44

rfiorella changed the title ~~[WIP] Refactor and clean up dapper; approved breaking changes and bug fixes~~ Refactor and clean up dapper; approved breaking changes and bug fixes Oct 16, 2026

rfiorella marked this pull request as ready for review October 16, 2026 21:18

rfiorella force-pushed the refactor/cleanup branch from 0528c0e to d66fa7a Compare October 16, 2026 21:19

rfiorella requested review from jonschwenk, jsta and matthewhoffman October 16, 2026 21:28

jsta approved these changes Oct 17, 2026

View reviewed changes

rfiorella changed the title ~~Refactor and clean up dapper; approved breaking changes and bug fixes~~ Refactor and clean up dapper, and some bug fixes Oct 18, 2026

jonschwenk approved these changes Oct 19, 2026

View reviewed changes

matthewhoffman approved these changes Oct 21, 2026

View reviewed changes

jsta merged commit b9a125f into main Oct 26, 2026
3 checks passed

jsta deleted the refactor/cleanup branch October 26, 2026 19:04

Navigation Menu

Repository navigation

Refactor and clean up dapper, and some bug fixes - #50

Refactor and clean up dapper, and some bug fixes#50
jsta merged 30 commits into
mainfrom
refactor/cleanup

rfiorella commented Sep 30, 2026 •

edited

Loading

Uh oh!

jonschwenk commented Sep 30, 2026

Uh oh!

rfiorella commented Oct 16, 2026

Uh oh!

jonschwenk commented Oct 19, 2026

Uh oh!

jonschwenk commented Oct 19, 2026

Uh oh!

matthewhoffman left a comment

Uh oh!

rfiorella commented Oct 22, 2026

Uh oh!

rfiorella commented Oct 26, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rfiorella commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Refactor (no behavior change)

Potentially breaking changes

Bug fixes (one commit each, each with a regression test)

Uh oh!

jonschwenk commented Sep 30, 2026

Uh oh!

rfiorella commented Oct 16, 2026

Uh oh!

jonschwenk commented Oct 19, 2026

Uh oh!

jonschwenk commented Oct 19, 2026

Uh oh!

matthewhoffman left a comment

Choose a reason for hiding this comment

Uh oh!

rfiorella commented Oct 22, 2026

Uh oh!

rfiorella commented Oct 26, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rfiorella commented Sep 30, 2026 •

edited

Loading