diff --git a/.claude/skills/CHANGELOG.md b/.claude/skills/CHANGELOG.md index 2b4ab2ba81..3ef6114980 100644 --- a/.claude/skills/CHANGELOG.md +++ b/.claude/skills/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed +- **`qsv-viz` redescribes `--location-mode` and drops its fixed `iso3` default** ([#4877](https://github.com/dathere/qsv/pull/4877)): the default now follows the region source, `geojson-id` when a `--geojson` is given (without `--geocode`), else `iso3`. A client that sent `--locations` + `--geojson` and relied on the default used to get a blank choropleth. - **`qsv-pivotp` redescribes `--maintain-order` and its input** ([#4871](https://github.com/dathere/qsv/pull/4871)): output is always in first-appearance order, so `--maintain-order` is accepted only for compatibility; `.sz` input/output, `QSV_COMMENT_CHAR`, `QSV_POLARS_FLOAT_PRECISION` and `QSV_OUTPUT_BOM` are documented. - **`qsv-pivotp` gains `--no-moarstats`** ([#4870](https://github.com/dathere/qsv/pull/4870)): with a single `--values` column, `--agg smart` now enriches the input's stats cache with `moarstats --advanced` (one extra pass over the file, reused afterwards); `--no-moarstats` skips it. The `--agg smart` description covers the new bimodality cut (0.60) and the positive-data-only ratios. - **`qsv-pivotp` redescribes `--agg smart` and `--delimiter`** ([#4869](https://github.com/dathere/qsv/pull/4869)): high variability now means CV above 100%, skewness comes from moment skewness (G1) with Pearson as the fallback, and a single-value column gets `first` instead of `item`; the description recommends an explicit `--agg` to pin results. `--delimiter` documents `QSV_DEFAULT_DELIMITER` and the `.tsv`/`.tab`/`.ssv` extensions. diff --git a/.claude/skills/qsv/qsv-viz.json b/.claude/skills/qsv/qsv-viz.json index e1419c03a6..f028f5960b 100644 --- a/.claude/skills/qsv/qsv-viz.json +++ b/.claude/skills/qsv/qsv-viz.json @@ -244,8 +244,7 @@ { "flag": "--location-mode", "type": "string", - "description": "How --locations values are matched to regions. One of: iso3 (the default, ISO-3166-1 alpha-3 country codes), usa-states (2-letter US state codes), country-names (full country names), geojson-id (match a --geojson feature id).", - "default": "iso3" + "description": "How --locations values are matched to regions. One of: iso3 (ISO-3166-1 alpha-3 country codes), usa-states (2-letter US state codes), country-names (full country names), geojson-id (match a --geojson feature id). Defaults to geojson-id when a --geojson source is given (without --geocode), else iso3." }, { "flag": "--locations", diff --git a/CHANGELOG.md b/CHANGELOG.md index 93b5bf3dec..73c62b8574 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Internal: `src/util.rs` split into 15 submodules under `src/util/`.** The 7,800-line file is now a facade that keeps the shared statics, `SchemaArgs` and `get_args`, and re-exports `stats_cache`, `input`, `special_format`, `http` and the other submodules, so every `util::…` path is unchanged. Pure code motion with no behavior change ([#4855](https://github.com/dathere/qsv/pull/4855)). ### Fixed +- **`viz choropleth --locations ... --geojson ...` no longer draws a blank map when `--location-mode` is omitted.** ([#4877](https://github.com/dathere/qsv/pull/4877)) The option defaulted to `iso3` even beside a `--geojson`, so the chart told plotly to look regions up as ISO-3 country codes. plotly then ignored the GeoJSON and shaded nothing, while qsv exited 0. This hit the help text's own `--geojson auto` example, `census:*` boundary shortcuts, and any GeoJSON keyed by feature id; only `--map` was unaffected. A supplied `--geojson` now implies `geojson-id` (except with `--geocode`, which yields ISO-3/US-state codes). An explicit `--location-mode` still wins, and without `--geojson` the default is still `iso3`. Related choropleth location fixes in the same PR: + - Location cells are trimmed in the built-in `iso3`/`usa-states` modes, so `PA ` and `PA` are one region instead of two, and `--denominator census` finds padded codes. + - A GeoJSON feature id stored as an integral float (`"FIPS": 6037.0`, as geopandas writes an integer column that holds a NaN) is emitted as `6037`, the spelling plotly matches, instead of `6037.0`, which matched no region. + - A mistyped `--denominator census@` (e.g. `census@2O21`) is now a clear usage error. It used to be read as a column name and fail with "selector does not exist" (on `viz choropleth`, only after fetching boundaries). - **`joinp --cache-schema 1` no longer writes a schema that types unparseable dates as dates.** ([#4872](https://github.com/dathere/qsv/pull/4872)) When it reused a stats cache that had inferred dates, as a `moarstats --advanced` cache (from `viz smart --smarter` or `pivotp --agg smart`) does, every Date/DateTime column became a Polars date type, even in formats Polars can't read such as `04/18/2019 09:55:45 PM`. The join failed, and the cached `.pschema.json` it left behind broke later `joinp` and `sqlp` reads of that file. As in `schema --polars` and `sqlp --cache-schema` since #4751, a column now gets a date type only if Polars parses a sample of its raw values; otherwise it is String. Day-first dates need `QSV_PREFER_DMY`. - **`pivotp --agg smart` (the default) picks Median far less often — behavior change.** ([#4869](https://github.com/dathere/qsv/pull/4869)) Its "high variability" test compared the stats cache's `cv` with 1, but `stats` reports CV as a percentage, so any numeric column with a CV above 1% got Median. That was nearly every one, and it hid the cardinality, mixed-sign and skewness checks after it. The cut is now 100% (stddev larger than the mean). **Pivots that relied on the default can now return Sum (or Mean) where they returned Median; pass an explicit `--agg` (e.g. `--agg median`) to keep the old result.** - **`pivotp --agg smart` now detects skewed data.** ([#4869](https://github.com/dathere/qsv/pull/4869)) Its skewness check read `stats`' `skewness`, which is Bowley's quartile skewness and never exceeds 1 in absolute value, against a cut of 2, so it could never fire. It now uses `moarstats --advanced`'s moment skewness (|G1| ≥ 1 picks Median), falling back to Pearson skewness when G1 isn't in the cache. diff --git a/docs/help/viz.md b/docs/help/viz.md index 6182918c5a..159fd31bd1 100644 --- a/docs/help/viz.md +++ b/docs/help/viz.md @@ -439,7 +439,7 @@ qsv viz --help |        Option         | Type | Description | Default | |--------|------|-------------|--------| |  `‑‑locations`  | string | Column holding the region key for each row (an ISO-3 country code, a 2-letter US state code, a country name, or a GeoJSON feature id, per --location-mode). With --geocode, this instead names a place-name column to forward-geocode into region codes. | | -|  `‑‑location‑mode`  | string | How --locations values are matched to regions. One of: iso3 (the default, ISO-3166-1 alpha-3 country codes), usa-states (2-letter US state codes), country-names (full country names), geojson-id (match a --geojson feature id). | `iso3` | +|  `‑‑location‑mode`  | string | How --locations values are matched to regions. One of: iso3 (ISO-3166-1 alpha-3 country codes), usa-states (2-letter US state codes), country-names (full country names), geojson-id (match a --geojson feature id). Defaults to geojson-id when a --geojson source is given (without --geocode), else iso3. | | |  `‑‑color‑scale`  | string | Colorscale for the region fill. One of: viridis (the default), cividis, greys, greens, blues, reds, ylgnbu, ylorrd, bluered, rdbu, portland, electric, jet, hot, blackbody, earth, picnic, rainbow. | `viridis` | |  `‑‑map`  | flag | Render on a token-free MapLibre tile basemap (a ChoroplethMap) instead of the default projection basemap. Requires --geojson; feature-id-key defaults to id. Reuses --style for the basemap. | | |  `‑‑geojson`  | string | Custom region polygons: a local file path, an http(s) URL to a GeoJSON FeatureCollection, or a shortcut name from the QSV_GEOJSON_SHORTCUTS env var (whose id sets --feature-id-key when you don't pass one). Use `auto` (or `census`) to fetch US boundaries from Census TIGERweb automatically; force a layer with census:state, census:county, census:zcta, census:tract or census:place (incorporated places AND CDPs), and pin a vintage with @, e.g. census:county@2021. That path sets feature-id-key to properties.GEOID (properties.STUSAB for a column of USPS state codes; full state names are matched to it) and caches under ~/.qsv-cache for 30 days (QSV_VIZ_BOUNDARY_CACHE_TTL_DAYS); it reads the codes from the --locations column, so in `viz smart` it needs a --dictionary naming the region column. A column of city/place NAMES also needs --geocode. Required for --map and for the geojson-id location mode; with --lat/--lon it instead enables point-in-polygon binning. See | | diff --git a/src/cmd/viz.rs b/src/cmd/viz.rs index f1a3f1c28f..d3b0def6be 100644 --- a/src/cmd/viz.rs +++ b/src/cmd/viz.rs @@ -376,9 +376,10 @@ choropleth options: feature id, per --location-mode). With --geocode, this instead names a place-name column to forward-geocode into region codes. --location-mode How --locations values are matched to regions. One of: iso3 - (the default, ISO-3166-1 alpha-3 country codes), usa-states - (2-letter US state codes), country-names (full country names), - geojson-id (match a --geojson feature id). [default: iso3] + (ISO-3166-1 alpha-3 country codes), usa-states (2-letter US + state codes), country-names (full country names), geojson-id + (match a --geojson feature id). Defaults to geojson-id when + a --geojson source is given (without --geocode), else iso3. --color-scale Colorscale for the region fill. One of: viridis (the default), cividis, greys, greens, blues, reds, ylgnbu, ylorrd, bluered, rdbu, portland, electric, jet, hot, blackbody, earth, picnic, @@ -1916,7 +1917,14 @@ pub fn run(argv: &[&str]) -> CliResult<()> { // `--denominator census` is read from the raw argv: docopt parses the flag into a // `SelectColumns`, which cannot tell the reserved source name from a column of that name. - let census_denominator = ArgvExtras::from_argv(argv).census_denominator.is_some(); + let argv_extras = ArgvExtras::from_argv(argv); + if argv_extras.bad_census_vintage { + return fail_incorrectusage_clierror!( + "--denominator census@: the year must be a four-digit ACS vintage (e.g. \ + census@2022)." + ); + } + let census_denominator = argv_extras.census_denominator.is_some(); // Denominator scope checks (issue #4394). Both flags name a per-region denominator that turns // a raw-count region map into a rate map; they are two SOURCES for the same quantity, so @@ -5549,7 +5557,7 @@ fn feature_id_by_path_value(feature: &serde_json::Value, path: &str) -> Option Some(s.clone()), - serde_json::Value::Number(n) => Some(n.to_string()), + serde_json::Value::Number(n) => Some(feature_id_number_string(n)), _ => None, } } @@ -7995,6 +8003,19 @@ fn feature_member_by_path<'a>( Some(cur) } +/// Stringify a numeric GeoJSON feature id the way plotly will see it. plotly keys its lookup by +/// JS `String(value)`, which prints an integral float as an integer (`6037.0` -> `"6037"`), while +/// `serde_json` prints `"6037.0"`. Integral floats are common: geopandas writes an integer code +/// column as float whenever it holds a `NaN`. Non-integral values keep `serde_json`'s spelling. +fn feature_id_number_string(n: &serde_json::Number) -> String { + match n.as_f64() { + Some(f) if n.is_f64() && f.is_finite() && f.fract() == 0.0 && f.abs() < 1e21 => { + format!("{f:.0}") + }, + _ => n.to_string(), + } +} + /// Resolve a GeoJSON feature's id by a dotted `--feature-id-key` path. Supports the top-level /// `"id"`, `"properties.<...>"` paths (mirroring plotly's `featureidkey` convention), and any /// other key as a top-level foreign member (e.g. a bare `"name"` on each feature — TopoJSON-style @@ -8005,12 +8026,12 @@ fn feature_id_by_path(feature: &geojson::Feature, key: &str) -> Option { if key == "id" { return feature.id.as_ref().map(|id| match id { geojson::feature::Id::String(s) => s.clone(), - geojson::feature::Id::Number(n) => n.to_string(), + geojson::feature::Id::Number(n) => feature_id_number_string(n), }); } match feature_member_by_path(feature, key)? { serde_json::Value::String(s) => Some(s.clone()), - serde_json::Value::Number(n) => Some(n.to_string()), + serde_json::Value::Number(n) => Some(feature_id_number_string(n)), _ => None, } } @@ -8634,6 +8655,10 @@ struct ArgvExtras { /// `--denominator census[@]`: the outer `Option` is "is this a Census request", the /// inner one the pinned vintage (issue #4395). census_denominator: Option>, + /// `--denominator census@` whose year does not parse. Without this, the value fails + /// `parse_census_denominator` and is resolved as a column NAME, which reports a misleading + /// error (and, on `viz choropleth`, only after fetching boundaries). + bad_census_vintage: bool, } impl ArgvExtras { @@ -8660,6 +8685,11 @@ impl ArgvExtras { "--denominator" => { extras.census_denominator = next_value.and_then(crate::cmd::viz_census::parse_census_denominator); + extras.bad_census_vintage = extras.census_denominator.is_none() + && next_value.is_some_and(|v| { + v.split_once('@') + .is_some_and(|(base, _)| base.eq_ignore_ascii_case("census")) + }); }, _ => {}, } @@ -10019,10 +10049,18 @@ fn build_choropleth_plot( // conjoined with --geocode — the county-name path publishes aliases without it. let region_aliases_active = auto_region_aliases().is_some(); let snap = !args.flag_no_snap; + // An omitted --location-mode follows the region source: a supplied --geojson keys regions by + // feature id. Defaulting to iso3 there emitted `locationmode: "ISO-3"` beside the geojson, and + // plotly then ignores the geojson and shades nothing. --geocode keeps iso3: it yields + // ISO-3/US-state codes, not feature ids. let mode = if pip || region_aliases_active { LocationMode::GeoJsonId + } else if let Some(m) = args.flag_location_mode.as_deref() { + parse_location_mode(m)? + } else if args.flag_geojson.is_some() && !args.flag_geocode { + LocationMode::GeoJsonId } else { - parse_location_mode(args.flag_location_mode.as_deref().unwrap_or("iso3"))? + LocationMode::Iso3 }; let palette = parse_color_scale(args.flag_color_scale.as_deref().unwrap_or("viridis"))?; @@ -10530,7 +10568,11 @@ fn choropleth_literal_locations( .unwrap_or_default(); let mut record = csv::ByteRecord::new(); while rdr.read_byte_record(&mut record)? { - let raw_loc = cell_to_string(record.get(loc_idx)); + // trimmed here, not only in the matcher: the built-in iso3/usa-states modes have no + // matcher, so `PA ` would otherwise aggregate apart from `PA` and match no plotly + // region (and miss a `--denominator census` value, which is keyed by the trimmed + // code) + let raw_loc = cell_to_string(record.get(loc_idx)).trim().to_string(); if raw_loc.is_empty() { continue; } diff --git a/tests/test_viz.rs b/tests/test_viz.rs index b0b11670e6..ce8d9f37e4 100644 --- a/tests/test_viz.rs +++ b/tests/test_viz.rs @@ -23800,3 +23800,168 @@ fn viz_smart_daemon_column_hook_typed_grid() { "no data-qsv-axis-columns map on the typed grid" ); } + +const TWO_SQUARES_BY_ID: &str = r#"{"type":"FeatureCollection","features":[ +{"type":"Feature","id":"A","properties":{},"geometry":{"type":"Polygon","coordinates":[[[0,0],[0,1],[1,1],[1,0],[0,0]]]}}, +{"type":"Feature","id":"B","properties":{},"geometry":{"type":"Polygon","coordinates":[[[2,0],[2,1],[3,1],[3,0],[2,0]]]}}]}"#; + +// `--locations` + `--geojson` with no `--location-mode` used to inherit docopt's `[default: iso3]`, +// emitting `"locationmode":"ISO-3"` beside the geojson. plotly then ignores the geojson and the map +// renders blank while exiting 0. A supplied --geojson must imply geojson-id. +#[test] +fn viz_choropleth_geojson_implies_geojson_id_location_mode() { + let wrk = Workdir::new("viz_choropleth_geojson_implies_geojson_id_location_mode"); + wrk.create_from_string("r.csv", "r,v\nA,1\nB,2\n"); + wrk.create_from_string("sq.geojson", TWO_SQUARES_BY_ID); + + let mut cmd = wrk.command("viz"); + cmd.args([ + "choropleth", + "r.csv", + "--locations", + "r", + "--value", + "v", + "--geojson", + "sq.geojson", + ]) + .env_remove("QSV_GEOJSON_SHORTCUTS"); + let out = wrk.output(&mut cmd); + assert!(out.status.success()); + let html = String::from_utf8_lossy(&out.stdout); + assert!( + html.contains(r#""locationmode":"geojson-id""#), + "a --geojson choropleth must key regions by feature id" + ); + assert!( + !html.contains(r#""locationmode":"ISO-3""#), + "ISO-3 beside a geojson makes plotly ignore the geojson (blank map)" + ); +} + +// An explicit --location-mode still wins over the geojson-implied default, and with no --geojson +// the default stays iso3. +#[test] +fn viz_choropleth_location_mode_default_without_geojson_and_explicit_override() { + let wrk = Workdir::new("viz_choropleth_location_mode_default_without_geojson"); + wrk.create_from_string("c.csv", "iso,v\nUSA,1\nCAN,2\n"); + wrk.create_from_string("sq.geojson", TWO_SQUARES_BY_ID); + + let mut cmd = wrk.command("viz"); + cmd.args(["choropleth", "c.csv", "--locations", "iso", "--value", "v"]); + let out = wrk.output(&mut cmd); + assert!(out.status.success()); + assert!(String::from_utf8_lossy(&out.stdout).contains(r#""locationmode":"ISO-3""#)); + + let mut cmd = wrk.command("viz"); + cmd.args([ + "choropleth", + "c.csv", + "--locations", + "iso", + "--value", + "v", + "--location-mode", + "iso3", + "--geojson", + "sq.geojson", + ]) + .env_remove("QSV_GEOJSON_SHORTCUTS"); + let out = wrk.output(&mut cmd); + assert!(out.status.success()); + assert!(String::from_utf8_lossy(&out.stdout).contains(r#""locationmode":"ISO-3""#)); +} + +// The built-in usa-states/iso3 modes have no RegionMatcher, so location cells must be trimmed on +// read: `PA ` used to aggregate apart from `PA` and match no plotly region. +#[test] +fn viz_choropleth_builtin_mode_trims_locations() { + let wrk = Workdir::new("viz_choropleth_builtin_mode_trims_locations"); + wrk.create_from_string("ws.csv", "state,n\nPA,3\nPA ,4\n NY,5\n"); + + let mut cmd = wrk.command("viz"); + cmd.args([ + "choropleth", + "ws.csv", + "--locations", + "state", + "--value", + "n", + "--location-mode", + "usa-states", + ]); + let out = wrk.output(&mut cmd); + assert!(out.status.success()); + let html = String::from_utf8_lossy(&out.stdout); + assert!( + html.contains(r#""locations":["PA","NY"]"#), + "padded codes were not trimmed" + ); + assert!(html.contains(r#""z":[7.0,5.0]"#), "PA rows were not merged"); +} + +// An integral float feature id (`"FIPS": 6037.0`, as geopandas writes an int column holding a NaN) +// must be emitted as plotly sees it — JS `String(6037.0)` is "6037" — not serde_json's "6037.0", +// which matches no feature. +#[test] +fn viz_choropleth_pip_integral_float_feature_id() { + let wrk = Workdir::new("viz_choropleth_pip_integral_float_feature_id"); + wrk.create_from_string( + "fid.geojson", + r#"{"type":"FeatureCollection","features":[ +{"type":"Feature","properties":{"FIPS":6037.0},"geometry":{"type":"Polygon","coordinates":[[[0,0],[1,0],[1,1],[0,1],[0,0]]]}}, +{"type":"Feature","properties":{"FIPS":6059.0},"geometry":{"type":"Polygon","coordinates":[[[2,0],[3,0],[3,1],[2,1],[2,0]]]}}]}"#, + ); + wrk.create_from_string("fid.csv", "lat,lon\n0.5,0.5\n0.5,2.5\n0.4,0.4\n"); + + let mut cmd = wrk.command("viz"); + cmd.args([ + "choropleth", + "fid.csv", + "--lat", + "lat", + "--lon", + "lon", + "--geojson", + "fid.geojson", + "--feature-id-key", + "properties.FIPS", + ]) + .env_remove("QSV_GEOJSON_SHORTCUTS"); + let out = wrk.output(&mut cmd); + assert!(out.status.success()); + let html = String::from_utf8_lossy(&out.stdout); + assert!( + html.contains(r#""locations":["6037","6059"]"#), + "integral float ids must be emitted as JS would stringify them" + ); +} + +// A mistyped vintage (`census@2O21`, letter O) used to fall through to column resolution and fail +// with a misleading "selector does not exist" — on choropleth only after fetching boundaries. +#[test] +fn viz_denominator_census_bad_vintage_is_usage_error() { + let wrk = Workdir::new("viz_denominator_census_bad_vintage_is_usage_error"); + wrk.create_from_string("r.csv", "r,v\nA,1\nB,2\n"); + wrk.create_from_string("sq.geojson", TWO_SQUARES_BY_ID); + + let mut cmd = wrk.command("viz"); + cmd.args([ + "choropleth", + "r.csv", + "--locations", + "r", + "--geojson", + "sq.geojson", + "--denominator", + "census@2O21", + ]) + .env_remove("QSV_GEOJSON_SHORTCUTS"); + let out = cmd.output().unwrap(); + let got = String::from_utf8_lossy(&out.stderr); + assert!(!out.status.success(), "a bad vintage must fail the run"); + assert!( + got.contains("four-digit ACS vintage"), + "expected an invalid-vintage error; got: {got}" + ); +}