Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 179 additions & 0 deletions src/execute/position.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ pub fn apply_position_adjustments(
#[cfg(test)]
mod tests {
use super::*;
use crate::plot::facet::{Facet, FacetLayout};
use crate::plot::layer::{Geom, Position};
use crate::plot::{AestheticValue, Mappings, ParameterValue, Scale, ScaleType};
use polars::prelude::*;
Expand Down Expand Up @@ -322,4 +323,182 @@ mod tests {
assert!((-0.3..=0.3).contains(&v));
}
}

#[test]
fn test_stack_resets_per_facet_panel() {
// Stacking should compute independently within each facet panel.
// Without this, bars in the second facet panel stack on top of
// cumulative values from the first panel (see issue #244).
//
// Two facet panels (F1, F2) each with the same x="A" and two
// fill groups (X, Y). Stacking within each panel should start from 0.
let df = df! {
"__ggsql_aes_pos1__" => ["A", "A", "A", "A"],
"__ggsql_aes_pos2__" => [10.0, 20.0, 30.0, 40.0],
"__ggsql_aes_pos2end__" => [0.0, 0.0, 0.0, 0.0],
"__ggsql_aes_fill__" => ["X", "Y", "X", "Y"],
"__ggsql_aes_facet1__" => ["F1", "F1", "F2", "F2"],
}
.unwrap();

let mut layer = crate::plot::Layer::new(Geom::bar());
layer.mappings = {
let mut m = Mappings::new();
m.insert(
"pos1",
AestheticValue::standard_column("__ggsql_aes_pos1__"),
);
m.insert(
"pos2",
AestheticValue::standard_column("__ggsql_aes_pos2__"),
);
m.insert(
"pos2end",
AestheticValue::standard_column("__ggsql_aes_pos2end__"),
);
m.insert(
"fill",
AestheticValue::standard_column("__ggsql_aes_fill__"),
);
m.insert(
"facet1",
AestheticValue::standard_column("__ggsql_aes_facet1__"),
);
m
};
layer.partition_by = vec![
"__ggsql_aes_fill__".to_string(),
"__ggsql_aes_facet1__".to_string(),
];
layer.position = Position::stack();
layer.data_key = Some("__ggsql_layer_0__".to_string());

let mut spec = Plot::new();
spec.scales.push(make_discrete_scale("pos1"));
spec.scales.push(make_continuous_scale("pos2"));
spec.facet = Some(Facet::new(FacetLayout::Wrap {
variables: vec!["facet_var".to_string()],
}));
let mut data_map = HashMap::new();
data_map.insert("__ggsql_layer_0__".to_string(), df);

let mut spec_with_layer = spec;
spec_with_layer.layers.push(layer);

apply_position_adjustments(&mut spec_with_layer, &mut data_map).unwrap();

let result_df = data_map.get("__ggsql_layer_0__").unwrap();

// Sort by facet then fill so we can assert in predictable order
let result_df = result_df
.clone()
.lazy()
.sort_by_exprs(
[col("__ggsql_aes_facet1__"), col("__ggsql_aes_fill__")],
SortMultipleOptions::default(),
)
.collect()
.unwrap();

let pos2 = result_df
.column("__ggsql_aes_pos2__")
.unwrap()
.f64()
.unwrap();
let pos2end = result_df
.column("__ggsql_aes_pos2end__")
.unwrap()
.f64()
.unwrap();

let pos2_vals: Vec<f64> = pos2.into_iter().flatten().collect();
let pos2end_vals: Vec<f64> = pos2end.into_iter().flatten().collect();

// Expected (sorted by facet, fill):
// F1/X: pos2end=0, pos2=10 (first in panel, starts at 0)
// F1/Y: pos2end=10, pos2=30 (stacks on X)
// F2/X: pos2end=0, pos2=30 (first in panel, should reset to 0)
// F2/Y: pos2end=30, pos2=70 (stacks on X)
assert_eq!(
pos2end_vals[2], 0.0,
"F2 panel first bar should start at 0, not carry over from F1. pos2end={:?}, pos2={:?}",
pos2end_vals, pos2_vals
);
}

#[test]
fn test_dodge_ignores_facet_columns_in_group_count() {
// Dodge should compute n_groups per facet panel, not globally.
// With fill=["X","Y"] and facet=["F1","F2"], dodge should see
// 2 groups (X, Y) not 4 (X-F1, X-F2, Y-F1, Y-F2).
//
// With 2 groups and default width 0.9:
// adjusted_width = 0.9 / 2 = 0.45
// offsets: -0.225 (group X), +0.225 (group Y)
//
// If facet columns incorrectly inflate n_groups to 4:
// adjusted_width = 0.9 / 4 = 0.225
// offsets would be different (spread across 4 positions)
let df = df! {
"__ggsql_aes_pos1__" => ["A", "A", "A", "A"],
"__ggsql_aes_pos2__" => [10.0, 20.0, 30.0, 40.0],
"__ggsql_aes_pos2end__" => [0.0, 0.0, 0.0, 0.0],
"__ggsql_aes_fill__" => ["X", "Y", "X", "Y"],
"__ggsql_aes_facet1__" => ["F1", "F1", "F2", "F2"],
}
.unwrap();

let mut layer = crate::plot::Layer::new(Geom::bar());
layer.mappings = {
let mut m = Mappings::new();
m.insert(
"pos1",
AestheticValue::standard_column("__ggsql_aes_pos1__"),
);
m.insert(
"pos2",
AestheticValue::standard_column("__ggsql_aes_pos2__"),
);
m.insert(
"pos2end",
AestheticValue::standard_column("__ggsql_aes_pos2end__"),
);
m.insert(
"fill",
AestheticValue::standard_column("__ggsql_aes_fill__"),
);
m.insert(
"facet1",
AestheticValue::standard_column("__ggsql_aes_facet1__"),
);
m
};
layer.partition_by = vec![
"__ggsql_aes_fill__".to_string(),
"__ggsql_aes_facet1__".to_string(),
];
layer.position = Position::dodge();
layer.data_key = Some("__ggsql_layer_0__".to_string());

let mut spec = Plot::new();
spec.scales.push(make_discrete_scale("pos1"));
spec.scales.push(make_continuous_scale("pos2"));
spec.facet = Some(Facet::new(FacetLayout::Wrap {
variables: vec!["facet_var".to_string()],
}));
let mut data_map = HashMap::new();
data_map.insert("__ggsql_layer_0__".to_string(), df);

spec.layers.push(layer);

apply_position_adjustments(&mut spec, &mut data_map).unwrap();

// With 2 groups (X, Y), adjusted_width should be 0.45
let adjusted = spec.layers[0].adjusted_width.unwrap();
assert!(
(adjusted - 0.45).abs() < 0.001,
"adjusted_width should be 0.45 (2 groups), got {} (facet columns inflated group count)",
adjusted
);
}
}
11 changes: 8 additions & 3 deletions src/plot/layer/position/dodge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
//! - If only pos2 is discrete → dodge vertically (pos2offset)
//! - If both are discrete → 2D grid dodge (both offsets, arranged in a grid)

use super::{compute_dodge_offsets, is_continuous_scale, Layer, PositionTrait, PositionType};
use super::{
compute_dodge_offsets, is_continuous_scale, non_facet_partition_cols, Layer, PositionTrait,
PositionType,
};
use crate::plot::types::{DefaultParamValue, ParamConstraint, ParamDefinition, ParameterValue};
use crate::{naming, DataFrame, GgsqlError, Plot, Result};
use polars::prelude::*;
Expand Down Expand Up @@ -159,8 +162,10 @@ fn apply_dodge_with_width(
return Ok((df, None));
}

// Compute group indices
let group_info = match compute_group_indices(&df, &layer.partition_by)? {
// Compute group indices, excluding facet columns so group count
// reflects within-panel groups (not cross-panel composites)
let group_cols = non_facet_partition_cols(&layer.partition_by, spec);
let group_info = match compute_group_indices(&df, &group_cols)? {
Some(info) => info,
None => return Ok((df, None)),
};
Expand Down
10 changes: 6 additions & 4 deletions src/plot/layer/position/jitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
//! - `normal`: normal/Gaussian distribution with ~95% of points within the width

use super::{
compute_dodge_offsets, compute_group_indices, is_continuous_scale, Layer, PositionTrait,
PositionType,
compute_dodge_offsets, compute_group_indices, is_continuous_scale, non_facet_partition_cols,
Layer, PositionTrait, PositionType,
};
use crate::plot::types::{DefaultParamValue, ParamConstraint, ParamDefinition, ParameterValue};
use crate::{naming, DataFrame, GgsqlError, Plot, Result};
Expand Down Expand Up @@ -491,9 +491,11 @@ fn apply_jitter(df: DataFrame, layer: &Layer, spec: &Plot) -> Result<DataFrame>
let mut rng = rand::thread_rng();
let n_rows = df.height();

// Compute group info for dodge-first behavior
// Compute group info for dodge-first behavior, excluding facet columns
// so group count reflects within-panel groups
let group_cols = non_facet_partition_cols(&layer.partition_by, spec);
let group_info = if dodge {
compute_group_indices(&df, &layer.partition_by)?
compute_group_indices(&df, &group_cols)?
} else {
None
};
Expand Down
26 changes: 26 additions & 0 deletions src/plot/layer/position/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,32 @@ pub fn compute_dodge_offsets(
}
}

/// Filter facet columns out of partition_by for position adjustments that
/// compute group indices (dodge, jitter).
///
/// Facet columns in partition_by inflate the group count — e.g., 2 fill groups
/// across 2 facet panels would be seen as 4 composite groups instead of 2.
/// Position adjustments should operate per-panel, so facet columns must be excluded.
pub fn non_facet_partition_cols(partition_by: &[String], spec: &Plot) -> Vec<String> {
let facet_cols: std::collections::HashSet<String> = spec
.facet
.as_ref()
.map(|f| {
f.layout
.internal_facet_names()
.into_iter()
.map(|aes| crate::naming::aesthetic_column(&aes))
.collect()
})
.unwrap_or_default();

partition_by
.iter()
.filter(|col| !facet_cols.contains(*col))
.cloned()
.collect()
}

// Re-export position implementations
pub use dodge::{compute_group_indices, Dodge, GroupIndices};
pub use identity::Identity;
Expand Down
19 changes: 15 additions & 4 deletions src/plot/layer/position/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,22 +266,33 @@ fn apply_stack(df: DataFrame, layer: &Layer, spec: &Plot, mode: StackMode) -> Re
// 2. stack_end_col = lag(stack_col, 1, 0) - the bar bottom/start (previous stack top)
// The cumsum naturally stacks across the grouping column values

// Build the partition columns for .over(): group column + facet columns.
// Facet columns must be included so stacking resets per facet panel,
// matching ggplot2 where position adjustments are computed per-panel.
let mut over_cols: Vec<Expr> = vec![col(&group_col)];
if let Some(ref facet) = spec.facet {
for aes in facet.layout.internal_facet_names() {
let facet_col = naming::aesthetic_column(&aes);
over_cols.push(col(&facet_col));
}
}

// Treat NA heights as 0 for stacking
// Compute cumulative sums (shared by all modes)
let lf = lf
.with_column(col(&stack_col).fill_null(lit(0.0)).alias(&stack_col))
.with_column(
col(&stack_col)
.cum_sum(false)
.over([col(&group_col)])
.over(&over_cols)
.alias("__cumsum__"),
)
.with_column(
col(&stack_col)
.cum_sum(false)
.shift(lit(1))
.fill_null(lit(0.0))
.over([col(&group_col)])
.over(&over_cols)
.alias("__cumsum_lag__"),
);

Expand All @@ -293,15 +304,15 @@ fn apply_stack(df: DataFrame, layer: &Layer, spec: &Plot, mode: StackMode) -> Re
vec!["__cumsum__", "__cumsum_lag__"],
),
StackMode::Fill(target) => {
let total = col(&stack_col).sum().over([col(&group_col)]);
let total = col(&stack_col).sum().over(&over_cols);
(
(col("__cumsum__") / total.clone() * lit(target)).alias(&stack_col),
(col("__cumsum_lag__") / total * lit(target)).alias(&stack_end_col),
vec!["__cumsum__", "__cumsum_lag__"],
)
}
StackMode::Center => {
let half_total = col(&stack_col).sum().over([col(&group_col)]) / lit(2.0);
let half_total = col(&stack_col).sum().over(&over_cols) / lit(2.0);
(
(col("__cumsum__") - half_total.clone()).alias(&stack_col),
(col("__cumsum_lag__") - half_total).alias(&stack_end_col),
Expand Down
Loading