Skip to content

Commit 7c5011d

Browse files
cpsievertclaude
andcommitted
fix: exclude facet columns from dodge/jitter group computation
Dodge and jitter used all partition_by columns (including facet columns) when computing group indices via compute_group_indices(). This inflated n_groups — e.g., 2 fill groups across 2 facet panels were seen as 4 composite groups, making bars too narrow and offsets incorrect. Add non_facet_partition_cols() helper that filters facet columns out of partition_by using spec.facet, and use it in both dodge and jitter. Closes #254 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent ff2622f commit 7c5011d

4 files changed

Lines changed: 116 additions & 7 deletions

File tree

src/execute/position.rs

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -425,4 +425,80 @@ mod tests {
425425
pos2end_vals, pos2_vals
426426
);
427427
}
428+
429+
#[test]
430+
fn test_dodge_ignores_facet_columns_in_group_count() {
431+
// Dodge should compute n_groups per facet panel, not globally.
432+
// With fill=["X","Y"] and facet=["F1","F2"], dodge should see
433+
// 2 groups (X, Y) not 4 (X-F1, X-F2, Y-F1, Y-F2).
434+
//
435+
// With 2 groups and default width 0.9:
436+
// adjusted_width = 0.9 / 2 = 0.45
437+
// offsets: -0.225 (group X), +0.225 (group Y)
438+
//
439+
// If facet columns incorrectly inflate n_groups to 4:
440+
// adjusted_width = 0.9 / 4 = 0.225
441+
// offsets would be different (spread across 4 positions)
442+
let df = df! {
443+
"__ggsql_aes_pos1__" => ["A", "A", "A", "A"],
444+
"__ggsql_aes_pos2__" => [10.0, 20.0, 30.0, 40.0],
445+
"__ggsql_aes_pos2end__" => [0.0, 0.0, 0.0, 0.0],
446+
"__ggsql_aes_fill__" => ["X", "Y", "X", "Y"],
447+
"__ggsql_aes_facet1__" => ["F1", "F1", "F2", "F2"],
448+
}
449+
.unwrap();
450+
451+
let mut layer = crate::plot::Layer::new(Geom::bar());
452+
layer.mappings = {
453+
let mut m = Mappings::new();
454+
m.insert(
455+
"pos1",
456+
AestheticValue::standard_column("__ggsql_aes_pos1__"),
457+
);
458+
m.insert(
459+
"pos2",
460+
AestheticValue::standard_column("__ggsql_aes_pos2__"),
461+
);
462+
m.insert(
463+
"pos2end",
464+
AestheticValue::standard_column("__ggsql_aes_pos2end__"),
465+
);
466+
m.insert(
467+
"fill",
468+
AestheticValue::standard_column("__ggsql_aes_fill__"),
469+
);
470+
m.insert(
471+
"facet1",
472+
AestheticValue::standard_column("__ggsql_aes_facet1__"),
473+
);
474+
m
475+
};
476+
layer.partition_by = vec![
477+
"__ggsql_aes_fill__".to_string(),
478+
"__ggsql_aes_facet1__".to_string(),
479+
];
480+
layer.position = Position::dodge();
481+
layer.data_key = Some("__ggsql_layer_0__".to_string());
482+
483+
let mut spec = Plot::new();
484+
spec.scales.push(make_discrete_scale("pos1"));
485+
spec.scales.push(make_continuous_scale("pos2"));
486+
spec.facet = Some(Facet::new(FacetLayout::Wrap {
487+
variables: vec!["facet_var".to_string()],
488+
}));
489+
let mut data_map = HashMap::new();
490+
data_map.insert("__ggsql_layer_0__".to_string(), df);
491+
492+
spec.layers.push(layer);
493+
494+
apply_position_adjustments(&mut spec, &mut data_map).unwrap();
495+
496+
// With 2 groups (X, Y), adjusted_width should be 0.45
497+
let adjusted = spec.layers[0].adjusted_width.unwrap();
498+
assert!(
499+
(adjusted - 0.45).abs() < 0.001,
500+
"adjusted_width should be 0.45 (2 groups), got {} (facet columns inflated group count)",
501+
adjusted
502+
);
503+
}
428504
}

src/plot/layer/position/dodge.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@
66
//! - If only pos2 is discrete → dodge vertically (pos2offset)
77
//! - If both are discrete → 2D grid dodge (both offsets, arranged in a grid)
88
9-
use super::{compute_dodge_offsets, is_continuous_scale, Layer, PositionTrait, PositionType};
9+
use super::{
10+
compute_dodge_offsets, is_continuous_scale, non_facet_partition_cols, Layer, PositionTrait,
11+
PositionType,
12+
};
1013
use crate::plot::types::{DefaultParamValue, ParamConstraint, ParamDefinition, ParameterValue};
1114
use crate::{naming, DataFrame, GgsqlError, Plot, Result};
1215
use polars::prelude::*;
@@ -159,8 +162,10 @@ fn apply_dodge_with_width(
159162
return Ok((df, None));
160163
}
161164

162-
// Compute group indices
163-
let group_info = match compute_group_indices(&df, &layer.partition_by)? {
165+
// Compute group indices, excluding facet columns so group count
166+
// reflects within-panel groups (not cross-panel composites)
167+
let group_cols = non_facet_partition_cols(&layer.partition_by, spec);
168+
let group_info = match compute_group_indices(&df, &group_cols)? {
164169
Some(info) => info,
165170
None => return Ok((df, None)),
166171
};

src/plot/layer/position/jitter.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@
1515
//! - `normal`: normal/Gaussian distribution with ~95% of points within the width
1616
1717
use super::{
18-
compute_dodge_offsets, compute_group_indices, is_continuous_scale, Layer, PositionTrait,
19-
PositionType,
18+
compute_dodge_offsets, compute_group_indices, is_continuous_scale, non_facet_partition_cols,
19+
Layer, PositionTrait, PositionType,
2020
};
2121
use crate::plot::types::{DefaultParamValue, ParamConstraint, ParamDefinition, ParameterValue};
2222
use crate::{naming, DataFrame, GgsqlError, Plot, Result};
@@ -491,9 +491,11 @@ fn apply_jitter(df: DataFrame, layer: &Layer, spec: &Plot) -> Result<DataFrame>
491491
let mut rng = rand::thread_rng();
492492
let n_rows = df.height();
493493

494-
// Compute group info for dodge-first behavior
494+
// Compute group info for dodge-first behavior, excluding facet columns
495+
// so group count reflects within-panel groups
496+
let group_cols = non_facet_partition_cols(&layer.partition_by, spec);
495497
let group_info = if dodge {
496-
compute_group_indices(&df, &layer.partition_by)?
498+
compute_group_indices(&df, &group_cols)?
497499
} else {
498500
None
499501
};

src/plot/layer/position/mod.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,32 @@ pub fn compute_dodge_offsets(
104104
}
105105
}
106106

107+
/// Filter facet columns out of partition_by for position adjustments that
108+
/// compute group indices (dodge, jitter).
109+
///
110+
/// Facet columns in partition_by inflate the group count — e.g., 2 fill groups
111+
/// across 2 facet panels would be seen as 4 composite groups instead of 2.
112+
/// Position adjustments should operate per-panel, so facet columns must be excluded.
113+
pub fn non_facet_partition_cols(partition_by: &[String], spec: &Plot) -> Vec<String> {
114+
let facet_cols: std::collections::HashSet<String> = spec
115+
.facet
116+
.as_ref()
117+
.map(|f| {
118+
f.layout
119+
.internal_facet_names()
120+
.into_iter()
121+
.map(|aes| crate::naming::aesthetic_column(&aes))
122+
.collect()
123+
})
124+
.unwrap_or_default();
125+
126+
partition_by
127+
.iter()
128+
.filter(|col| !facet_cols.contains(*col))
129+
.cloned()
130+
.collect()
131+
}
132+
107133
// Re-export position implementations
108134
pub use dodge::{compute_group_indices, Dodge, GroupIndices};
109135
pub use identity::Identity;

0 commit comments

Comments
 (0)