[WIP] Letid geospatial upgrades#351
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #351 +/- ##
===============================================
- Coverage 74.72% 73.85% -0.88%
===============================================
Files 41 41
Lines 4823 4911 +88
===============================================
+ Hits 3604 3627 +23
- Misses 1219 1284 +65 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
calc_letid_outdoors and calc_letid_lab integrate the 3-state A/B/C defect kinetics with an explicit forward-Euler step. Under extreme cold (e.g. NSRDB polar/Siberia winter, module temp below -60 C) the step overshoots, driving NA/NB to +/-1e5 and collapsing tau, so Pmp_norm crashes to ~0.56 -- the spurious dark band over polar Asia in the world LETID map. NA/NB/NC are percentages that conserve NA+NB+NC=100, so each is clamped to [0,100] after the Euler update. The clamp is a no-op for well-resolved, in-range steps (normal locations unchanged) and only projects overshoots back onto the physical range.
weather.py: black-wrap 3 lines >88 chars in nsrdb_gids/_downsample_time/get_NSRDB (flake8 E501 + black hook). tutorials/05_geospatial/03_letid_outdoor_geospatial_demo: jupytext --sync the notebook/script pair (script was stale: mechanism_params repins->D037, meta State column, gid chunking, dask account/interface) so the jupytext hook passes.
calc_gid forced the weather index to datetime64 with a bare np.asarray, which dropped the index name. Timeseries funcs (e.g. temperature.cell/module) inherit that unnamed index, so xr.Dataset.from_dataframe labelled the dim 'index' and the 'if not df.index.name' branch collapsed the whole series to a scalar -- dropping 'time' and making map_blocks raise 'Dimensions {time} missing on returned object'. Preserve the index name when forcing the dtype.
_df_from_arbitrary named a Series-result column after func.__name__, mismatching the auto/manual template's declared data variable (from @geospatial_quick_shape shape_names) and raising 'does not contain data variables {...}'. Name it after the declared shape name, and use rename().to_frame() instead of pd.DataFrame(res, columns=[name]) (which reindexes by label and silently yields an all-NaN column).
Verified: geospatial.analysis(func=temperature.cell) on the summit test data now returns (latitude, longitude, time); tests/test_geospatial.py passes 7/7.
Cell 12 accessed geo_temperature_res["module"], but the template in cell 10 (and temperature.module's @geospatial_quick_shape) declares the data variable "module_temperature"; the longitude was also a string with an exact .sel. Use "module_temperature" with .sel(latitude=, longitude=-106.42, method="nearest"). This cell was never reached in CI before because analysis(temperature.cell) failed earlier -- the geospatial.analysis fix (eb20993) unmasked it. The notebook now executes end-to-end (testbook).
There was a problem hiding this comment.
Generally, LGTM.
One small suggestion/question regarding the hardcoded directory.
It appears that the notebook was rerun in vscode, so I have tried running python TEMP/_strip_datawrangler.py tutorials/05_geospatial/03_letid_outdoor_geospatial_demo.ipynb to strip out the vs code-specific datawrangler metadata (doesn't affect the other outputs). This will reduce the diff and prevent the checks from being IDE specific. Not sure if you remember, but we discussed this vs code behaviour before when fixing a bunch of notebooks several versions ago... I couldn't find the PR at the moment, but I'll link it if it comes up.
If that commit messes things up, we can easily revert it.
| "processes": 104, | ||
| "interface": "hsn0", | ||
| "processes": 100, # workers per node; 104 single-thread procs overwhelm startup | ||
| "local_directory": "/scratch/mspringe/", # node-local scratch for fast worker startup |
There was a problem hiding this comment.
is it possible (necessary?) to make this generic/env based rather than hardcoding your personal path in this tutorial? For example:
| "local_directory": "/scratch/mspringe/", # node-local scratch for fast worker startup | |
| "local_directory": os.environ.get("TMPDIR") or f"/scratch/{os.environ.get('USER', '')}",``` |
There was a problem hiding this comment.
If you agree, this change can be committed here but the notebook will need to be updated separately (unless precommit fixes it?) because I cannot load the diff for the notebook on this PR since it's too large.
RDaxini
added
bug
RDaxini
left a comment
RDaxini
left a comment
There was a problem hiding this comment.
I have tried running
python TEMP/_strip_datawrangler.py tutorials/05_geospatial/03_letid_outdoor_geospatial_demo.ipynbto strip out the vs code-specific datawrangler metadata (doesn't affect the other outputs).
all checks still pass and stripping out the VS Code-specific metadata reduced the diff by over 1000 lines (and removes the IDE-specificity of the outputs). We could consider adding this into a pre-commit hook perhaps, in a future PR
3 participants
Describe your changes
Scales
geospatial.analysisto very large NSRDB grids and fixes several correctness bugs in the geospatial and LETID paths.Enhancements
weather.get(..., geospatial=True)/get_NSRDBnow acceptbbox,downsample,land_only, andresample, reading only the requested sites instead of the full grid. This makes huge grids (e.g. GOESfull_disc, ~9.5M sites) practical to load. Newweather.nsrdb_gidsresolves target GIDs from a fast coordinate‑only read.Polar(Arctic) NSRDB satellite and aland_onlyfilter (via the NSRDBoffshoreflag).geospatial.analysisgained agid_chunkargument and auto‑chunks alonggid, so single‑chunk inputs run in parallel instead of serially.Bug fixes
NA/NB/NCto[0, 100]incalc_letid_outdoors/calc_letid_lab; an explicit‑Euler step overshot at extreme cold (Arctic/Siberian winter) and crashed normalized power to ~0.56.timefor timeseriesSeries‑returning funcs (e.g.temperature.cell) →Dimensions {'time'} missing; preserve the weather index name and match the result variable to the declared template shape.nrel_kestrel_check→nlr_kestrel_check).Docs / tutorials
05_geospatialtutorials to run against the current API; added a pending changelog.Testing
tests/test_geospatial.pypasses (7/7); all geospatial/scenario testbook notebooks execute end‑to‑end.Want me to push the changelog commit and/or update the PR #351 body with this description?
Created 2 todos
Issue ticket number and link
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review