feat: add DeckGL gridded guidance and browser row-count guard#1825
Closed
ghostiee-11 wants to merge 12 commits into
Closed
feat: add DeckGL gridded guidance and browser row-count guard#1825ghostiee-11 wants to merge 12 commits into
ghostiee-11 wants to merge 12 commits into
Conversation
After holoviz#1791 landed, loading a NetCDF file and asking the AI to plot it produces a scatter or line chart rather than a 2D heatmap, because: - hvPlotBaseView had no way to pass a value column (C) to hvplot - The AI agents had no awareness the pipeline was backed by gridded xarray data Changes: - Add `C` param (value column) to hvPlotBaseView; wire into df.hvplot() - Add `quadmesh` to the kind selector - New lumen/ai/gridded.py: detect_gridded() returns coordinate/variable metadata when pipeline.source is XArraySQLSource, else None - BaseViewAgent._generate_yaml_spec: call detect_gridded() once, pass result as `gridded` template variable - BaseViewAgent/main.jinja2 + hvPlotAgent/main.jinja2: conditional block guides LLM to use kind='heatmap' with x/y/C for gridded data - hvPlotAgent._extract_spec: drop by/groupby when kind=heatmap Addresses holoviz#1821 (1/3)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1825 +/- ##
==========================================
+ Coverage 70.02% 70.30% +0.27%
==========================================
Files 193 197 +4
Lines 32445 32862 +417
==========================================
+ Hits 22721 23105 +384
- Misses 9724 9757 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
Looking good but I'm a little confused. Seems like the three PRs are overlapping to some degree. Could you combine them or indicate in what order I should review/merge them? |
Collaborator
Author
|
These three PRs are a stack, not independent changes. Each one adds a layer on top of the previous:
Review/merge order: 1823 → 1824 → 1825. After #1823 lands I will rebase #1824 onto main (which will make it a clean diff against #1823's merged state), same for #1825 after #1824 lands. If you'd prefer a single combined PR I'm happy to collapse them just let me know. |
Per review on holoviz#1824: hvPlot uses `C` only for heatmap (matplotlib hexbin convention), and `z` for image, quadmesh, contourf. Exposing the param as `C` for all gridded kinds would silently pass C= to plot kinds that expect z=. Fix: rename the public param `C` to `z` and map internally to C= only when kind='heatmap', otherwise pass through as z=. Updated prompt guidance in BaseViewAgent/main.jinja2 and hvPlotAgent/main.jinja2 to use z= and mention the other gridded kinds. Tests updated to use z= and a new test verifies the z-to-C mapping is conditional on kind.
ghostiee-11
force-pushed
the
feat/xarray-gridded-deckgl
branch
from
April 21, 2026 14:51
3f9d5be to
000bcac
Compare
Per review on holoviz#1823: heatmap treats x/y as ordinal categories, which is semantically wrong for continuous-coordinate gridded data. quadmesh and image preserve the continuous axes and are the right default for xarray- derived grids. But hvPlot's quadmesh/image/contourf kinds raise NotImplementedError on long-form pandas DataFrames (they expect a 2D array or xarray), and XArraySQLSource returns pandas via SQL. Fix: hvPlotBaseView.get_plot now pivots the long-form DataFrame to a 2D xarray DataArray (via set_index([y, x])[z].to_xarray()) when kind is in _GRIDDED_KINDS = (quadmesh, image, contourf) and x/y/z are set. Safety guards in _to_gridded: - Skips pivot if the input is not a pandas DataFrame (xarray objects pass through unchanged, so future xarray-returning sources work). - Skips pivot if hvplot.xarray failed to import (import flag set at __init__ time; avoids AttributeError at plot time). - Skips pivot if x/y/z are missing or required columns are absent (falls back to hvPlot's own NotImplementedError). - Skips pivot if duplicate (y, x) pairs exist (pivot would be ambiguous). - Raises ValueError if the pivoted grid would exceed _GRIDDED_MAX_CELLS = 10M cells (~80MB for float64) to prevent OOM. Prompt guidance updated: recommends kind='quadmesh' as the primary choice for xarray-derived gridded data, with image/contourf as alternatives and heatmap as a fallback for ordinal/categorical axes. heatmap still stays on the pandas path (it works fine there). hvplot.xarray is imported in hvPlotBaseView.__init__ alongside hvplot.pandas so the .hvplot accessor is available on DataArray. Tests (10 in test_hvplot_gridded.py): - test_hvplot_quadmesh_from_longform_df: renders hv.QuadMesh - test_hvplot_image_from_longform_df: renders hv.Image - test_hvplot_gridded_size_guard_raises: ValueError over cap - test_hvplot_gridded_skips_pivot_if_xarray_unavailable: flag fallback - test_hvplot_gridded_passes_through_non_dataframe: xarray input skip - existing z-param and heatmap tests updated
Pre-commit ruff auto-fix flagged the # noqa as unused; the try/except already silences the ImportError, so the # type: ignore is enough.
The previous lint cleanup over-stripped the noqa on hvplot.dask, which is a genuine F401 (conditional side-effect import for the .hvplot accessor on dask DataFrames). Restored as scoped # noqa: F401 so RUF100 recognises it as a targeted suppression.
ghostiee-11
force-pushed
the
feat/xarray-gridded-deckgl
branch
from
April 23, 2026 19:47
000bcac to
33763e6
Compare
Two review responses bundled: 1. Per review on holoviz#1824: BaseViewAgent/main.jinja2 is the base template inherited by hvPlotAgent, VegaLiteAgent, and DeckGLAgent. The previous gridded block named kind='quadmesh'/'image'/'contourf'/'heatmap' which are hvPlot-only concepts. VegaLite uses mark: rect, DeckGL uses layer types, so injecting hvPlot kind values into their prompt context is misleading. Kept the metadata (xarray source, dims, data_vars) in the base block; each agent's own prompt carries the syntax-specific guidance. Updated test to assert no hvPlot kinds leak into the base. 2. Style cleanup for lint discipline: moved MagicMock/patch/holoviews/ numpy/pandas imports to the top of test_hvplot_gridded.py (was inline inside test bodies). xarray is an optional dep, so it is guarded with pytest.importorskip inside the tests that actually use it to keep test-core collection working. Replaced two em dashes in hvPlotAgent prompt and a test docstring.
ghostiee-11
force-pushed
the
feat/xarray-gridded-deckgl
branch
2 times, most recently
from
April 23, 2026 20:04
233f5e8 to
b7c76ed
Compare
Per review on holoviz#1823: detect_gridded is a small one-function helper; it does not warrant its own module. Moved into lumen/ai/utils.py alongside the other AI helpers and removed the standalone gridded.py. Updated imports in lumen/ai/agents/base_view.py and lumen/tests/ai/test_gridded.py.
Inject gridded metadata into DeckGLAgent spec generation so the LLM receives dimension/data_var hints when the source is an XArraySQLSource. Add gridded guidance blocks to main.jinja2 and main_pydeck.jinja2 with ColumnLayer/HexagonLayer examples keyed to actual dim and data_var names. Raise ValueError in DeckGLView._get_params when row count exceeds _DECKGL_ROW_LIMIT (250,000) to prevent browser OOM on large grids. Add test_deckgl_gridded.py covering prompt rendering and row-limit guard. Addresses holoviz#1821 (3/3)
ghostiee-11
force-pushed
the
feat/xarray-gridded-deckgl
branch
from
April 24, 2026 12:24
b7c76ed to
7b50149
Compare
ghostiee-11
force-pushed
the
feat/xarray-gridded-deckgl
branch
from
April 24, 2026 12:27
7b50149 to
27fdd66
Compare
ahuang11
marked this pull request as draft
May 6, 2026 21:51
Collaborator
Author
|
Folding this into #1823, which includes the DeckGL gridded guidance and the DeckGLView 250k-row guard. Closing to keep it in a single reviewable PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a user loads an xarray dataset and asks for a DeckGL visualisation, the agent picks a generic scatter spec with no value encoding, producing an empty or broken map.
This adds a conditional block to the DeckGL prompt templates that fires only for xarray sources, steering the LLM toward
ColumnLayer/HexagonLayerwithgetPositionandgetElevationwired to the actual dimension and variable names. It also raises an explicitValueErrorinDeckGLView._get_paramswhen the frame exceeds 250,000 rows instead of silently OOMing the browser.How to reproduce
getElevationmapped to the value fieldHow Has This Been Tested?
Addresses #1821 (3/3)