diff --git a/doc/syntax/clause/facet.qmd b/doc/syntax/clause/facet.qmd index 6945386a..eb556f59 100644 --- a/doc/syntax/clause/facet.qmd +++ b/doc/syntax/clause/facet.qmd @@ -34,7 +34,7 @@ This clause behaves much like the `SETTINGS` clause in `DRAW`, in that it allows * `'y'`: Shared x-axis scale, independent y-axis scale * `['x', 'y']`: Independent scales for both axes * `missing`: Determines how layers behave when the faceting column is missing. It can take two values: `'repeat'` (default), and `'null'`. If `'repeat'` is set, then the layer data is repeated in each panel. If `'null'`, then such layers are only displayed if a null panel is shown, as controlled by the facet scale. -* `ncol`: The number of panel columns to use when faceting by a single variable. Default is 3 when fewer than 6 categories are present, 4 when fewer than 12 categeries are present and otherwise 5. When the `BY`-clause is used to set a second faceting variable, the `ncol` setting is not allowed. There is no `nrow` setting as this is derived from the number of panels and the `ncol` setting. +* `ncol`/`nrow`: The dimensions of the layout when faceting by a single variable. Only one of these can be given as the other is derived based on the number of panels to draw. Default is 3 columns when fewer than 6 categories are present, 4 columns when fewer than 12 categories are present and otherwise 5 columns. When the `BY`-clause is used to set a second faceting variable, the `ncol` an `nrow` setting are not allowed. ### Facet variables as aesthetics When you apply faceting to a plot you are creating new aesthetics you can control. For 1-dimensional faceting (no `BY` clause) the aesthetic is called `panel` and for 2-dimensional faceting the aesthetics are called `row` and `column`. You can read more about these aesthetics in [their documentation](../scale/aesthetic/Z_faceting.qmd) diff --git a/src/plot/facet/resolve.rs b/src/plot/facet/resolve.rs index 614c3ff9..29859ffd 100644 --- a/src/plot/facet/resolve.rs +++ b/src/plot/facet/resolve.rs @@ -51,7 +51,7 @@ impl FacetDataContext { } /// Allowed properties for wrap facets -const WRAP_ALLOWED: &[&str] = &["free", "ncol", "missing"]; +const WRAP_ALLOWED: &[&str] = &["free", "ncol", "nrow", "missing"]; /// Allowed properties for grid facets const GRID_ALLOWED: &[&str] = &["free", "missing"]; @@ -117,6 +117,11 @@ pub fn resolve_properties( "Setting `ncol` is only allowed for 1 dimensional facets, not 2 dimensional facets".to_string(), ); } + if key == "nrow" && !is_wrap { + return Err( + "Setting `nrow` is only allowed for 1 dimensional facets, not 2 dimensional facets".to_string(), + ); + } return Err(format!( "Unknown setting: '{}'. Allowed settings: {}", key, @@ -127,7 +132,7 @@ pub fn resolve_properties( // Step 2: Validate property values validate_free_property(facet, positional_names)?; - validate_ncol_property(facet)?; + validate_layout_properties(facet)?; validate_missing_property(facet)?; // Step 3: Normalize free property to boolean vector @@ -279,8 +284,22 @@ fn normalize_free_property(facet: &mut Facet, positional_names: &[&str]) { .insert("free".to_string(), ParameterValue::Array(bool_array)); } -/// Validate ncol property value -fn validate_ncol_property(facet: &Facet) -> Result<(), String> { +/// Validate ncol and nrow properties +/// +/// - Both must be positive integers if present +/// - They are mutually exclusive (cannot both be specified) +fn validate_layout_properties(facet: &Facet) -> Result<(), String> { + let has_ncol = facet.properties.contains_key("ncol"); + let has_nrow = facet.properties.contains_key("nrow"); + + // Check mutual exclusivity first + if has_ncol && has_nrow { + return Err( + "`ncol` and `nrow` cannot both be specified. Use one or the other.".to_string(), + ); + } + + // Validate ncol if present if let Some(value) = facet.properties.get("ncol") { match value { ParameterValue::Number(n) => { @@ -288,11 +307,22 @@ fn validate_ncol_property(facet: &Facet) -> Result<(), String> { return Err(format!("`ncol` must be a positive integer, got {}", n)); } } - _ => { - return Err("'ncol' must be a number".to_string()); + _ => return Err("'ncol' must be a number".to_string()), + } + } + + // Validate nrow if present + if let Some(value) = facet.properties.get("nrow") { + match value { + ParameterValue::Number(n) => { + if *n <= 0.0 || n.fract() != 0.0 { + return Err(format!("`nrow` must be a positive integer, got {}", n)); + } } + _ => return Err("'nrow' must be a number".to_string()), } } + Ok(()) } @@ -322,13 +352,29 @@ fn apply_defaults(facet: &mut Facet, context: &FacetDataContext) { // Note: absence of 'free' property means fixed/shared scales (default) // No need to insert a default value - // Default ncol for wrap facets (computed from data) - if facet.is_wrap() && !facet.properties.contains_key("ncol") { - let default_cols = compute_default_ncol(context.num_levels); - facet.properties.insert( - "ncol".to_string(), - ParameterValue::Number(default_cols as f64), - ); + // Handle ncol/nrow for wrap facets + if facet.is_wrap() { + let has_ncol = facet.properties.contains_key("ncol"); + let has_nrow = facet.properties.contains_key("nrow"); + + if has_nrow && !has_ncol { + // User provided nrow: compute ncol from it + if let Some(ParameterValue::Number(nrow)) = facet.properties.get("nrow") { + let nrow_val = *nrow as usize; + let ncol = ((context.num_levels as f64) / (nrow_val as f64)).ceil() as i64; + facet + .properties + .insert("ncol".to_string(), ParameterValue::Number(ncol as f64)); + facet.properties.remove("nrow"); + } + } else if !has_ncol && !has_nrow { + // Neither provided: apply default ncol + let default_cols = compute_default_ncol(context.num_levels); + facet.properties.insert( + "ncol".to_string(), + ParameterValue::Number(default_cols as f64), + ); + } } } @@ -903,4 +949,184 @@ mod tests { assert!(err.contains("'theta'")); assert!(err.contains("'x'") || err.contains("'y'")); } + + // ======================================== + // nrow Property Tests + // ======================================== + + #[test] + fn test_nrow_computes_ncol() { + // 10 levels, nrow=2 -> ncol=5 + let mut facet = make_wrap_facet(); + facet + .properties + .insert("nrow".to_string(), ParameterValue::Number(2.0)); + + let context = make_context(10); + resolve_properties(&mut facet, &context, CARTESIAN).unwrap(); + + // nrow should be removed and ncol computed + assert!(!facet.properties.contains_key("nrow")); + assert_eq!( + facet.properties.get("ncol"), + Some(&ParameterValue::Number(5.0)) + ); + } + + #[test] + fn test_nrow_with_remainder() { + // 10 levels, nrow=3 -> ncol=ceil(10/3)=4 + let mut facet = make_wrap_facet(); + facet + .properties + .insert("nrow".to_string(), ParameterValue::Number(3.0)); + + let context = make_context(10); + resolve_properties(&mut facet, &context, CARTESIAN).unwrap(); + + assert!(!facet.properties.contains_key("nrow")); + assert_eq!( + facet.properties.get("ncol"), + Some(&ParameterValue::Number(4.0)) + ); + } + + #[test] + fn test_nrow_larger_than_num_levels() { + // 3 levels, nrow=10 -> ncol=ceil(3/10)=1 + let mut facet = make_wrap_facet(); + facet + .properties + .insert("nrow".to_string(), ParameterValue::Number(10.0)); + + let context = make_context(3); + resolve_properties(&mut facet, &context, CARTESIAN).unwrap(); + + assert!(!facet.properties.contains_key("nrow")); + assert_eq!( + facet.properties.get("ncol"), + Some(&ParameterValue::Number(1.0)) + ); + } + + #[test] + fn test_nrow_equals_num_levels() { + // 5 levels, nrow=5 -> ncol=ceil(5/5)=1 + let mut facet = make_wrap_facet(); + facet + .properties + .insert("nrow".to_string(), ParameterValue::Number(5.0)); + + let context = make_context(5); + resolve_properties(&mut facet, &context, CARTESIAN).unwrap(); + + assert!(!facet.properties.contains_key("nrow")); + assert_eq!( + facet.properties.get("ncol"), + Some(&ParameterValue::Number(1.0)) + ); + } + + #[test] + fn test_error_ncol_and_nrow_both_provided() { + let mut facet = make_wrap_facet(); + facet + .properties + .insert("ncol".to_string(), ParameterValue::Number(3.0)); + facet + .properties + .insert("nrow".to_string(), ParameterValue::Number(2.0)); + + let context = make_context(10); + let result = resolve_properties(&mut facet, &context, CARTESIAN); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.contains("ncol")); + assert!(err.contains("nrow")); + assert!(err.contains("cannot both be specified")); + } + + #[test] + fn test_error_negative_nrow() { + let mut facet = make_wrap_facet(); + facet + .properties + .insert("nrow".to_string(), ParameterValue::Number(-1.0)); + + let context = make_context(5); + let result = resolve_properties(&mut facet, &context, CARTESIAN); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.contains("nrow")); + assert!(err.contains("positive")); + } + + #[test] + fn test_error_non_integer_nrow() { + let mut facet = make_wrap_facet(); + facet + .properties + .insert("nrow".to_string(), ParameterValue::Number(2.5)); + + let context = make_context(5); + let result = resolve_properties(&mut facet, &context, CARTESIAN); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.contains("nrow")); + assert!(err.contains("integer")); + } + + #[test] + fn test_error_nrow_on_grid() { + let mut facet = make_grid_facet(); + facet + .properties + .insert("nrow".to_string(), ParameterValue::Number(2.0)); + + let context = make_context(10); + let result = resolve_properties(&mut facet, &context, CARTESIAN); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.contains("nrow")); + assert!(err.contains("1 dimensional")); + } + + #[test] + fn test_nrow_not_string() { + let mut facet = make_wrap_facet(); + facet.properties.insert( + "nrow".to_string(), + ParameterValue::String("2".to_string()), + ); + + let context = make_context(5); + let result = resolve_properties(&mut facet, &context, CARTESIAN); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.contains("nrow")); + assert!(err.contains("number")); + } + + #[test] + fn test_user_ncol_preserved() { + // Existing behavior: user-provided ncol should be preserved + let mut facet = make_wrap_facet(); + facet + .properties + .insert("ncol".to_string(), ParameterValue::Number(2.0)); + + let context = make_context(10); + resolve_properties(&mut facet, &context, CARTESIAN).unwrap(); + + // User's ncol should be preserved, not overwritten + assert_eq!( + facet.properties.get("ncol"), + Some(&ParameterValue::Number(2.0)) + ); + } }