feat(histogram): add faceted plots with adaptive layout by zhan4329 · Pull Request #428 · FNLCR-DMAP/SCSAWorkflow

zhan4329 · 2026-04-17T03:01:25Z

Related PR

This PR depends on PR #328.

Summary

This PR finalizes histogram facet support in SCSAWorkflow by adding a faceted grouped-histogram path in spac.visualization.histogram() and aligning histogram_template.py with the same facet/grouped contract.

Changes

Adds facet=True grouped histogram support with shared bins and shared category slots, adaptive facet geometry, figure-level labels, grouped count-table returns, and grouped-mode guardrails.
Aligns template-side validation and forwarding for Facet, Facet_Ncol, Max_Groups, facet size hints, and overlay-only multiple, while keeping grouped-only and facet-only hints inactive outside their modes.
Adds focused tests for _derive_facet_geometry(), facet behavior, bins fallback, shared-bin consistency, max_groups, and external-ax guardrails.

Testing

pytest tests/test_visualization/test_histogram.py tests/test_visualization/test_derive_facet_geometry.py tests/templates/test_histogram_template.py -q
54 passed

Notes For Review

Suggested review order:
1. src/spac/visualization.py for the facet plotting path, shared-bin behavior, and grouped return-data contract.
2. src/spac/templates/histogram_template.py for template-side validation, forwarding, and layout handling.
3. tests/test_visualization/test_histogram.py, tests/test_visualization/test_derive_facet_geometry.py, and tests/templates/test_histogram_template.py for focused coverage.
More implementation details can be found here: pr-details.md.
Here is a jupyter notebook with plenty of tests: test_histogram_facet_light_template.ipynb. Some output figures are attached below.
Here is a list of possible future work: future-work.md

This pull request body is generated with the help of Codex using GPT-5.4 (xhigh) and verified by me

Selected Figures

Existing Grouped-Together Path

Existing Grouped-Separate Path (not consistent)

New Faceted Path (consistent scale)

Existing Grouped-Seperate Path with Long Labels (overlap)

New Faceted Path with Long Labels (good)

New Faceted Path with More Groups

- Add customizable FacetGrid parameters: facet_ncol, facet_vertical_threshold, facet_height, and facet_aspect - Implement automatic grid layout that switches between vertical (<=4 groups) and compact grid layout (>4 groups) - Improve visual styling with background color and grid lines on facets - Fix axis label handling for faceted plots using supxlabel/supylabel - Adjust margins and spacing for better readability across different layouts - Update docstring with new FacetGrid customization parameters

…ility

- Add TODO comments documenting the need to review binning logic in the histogram function. Notes indicate potential double-binning behavior that may need refactoring to pass global_bin_edges to seaborn directly. - Also adds clarifying comment about figure and axes output.

…l_bin_edges - Add new helper function _compute_global_bin_edges() to consolidate consistent bin-edge calculation for numeric and categorical data - Replace duplicate bin-edge logic in together and facet cases - Improves code maintainability and reduces duplication in histogram workflow

…pecification

- extract helpers for internal layout kwarg parsing, facet geometry derivation, and axis label resolution - simplify histogram flow by removing duplicated label logic and centralizing kwargs sanitization - keep facet API boundary clean by documenting only facet_ncol as user-facing and target_fig_width/target_fig_height as internal hints

- add Facet and Facet_Ncol parsing and validation in template - normalize facet_ncol handling in histogram layout kwargs - adjust facet_ncol hints accordingly in histogram docstrings

- expose Element in histogram template inputs - validate multiple, element, and stat values in template - document shrink and alpha keyword behavior in histogram docstring

- force `multiple="dodge"` when `together=False` in histogram template - reject invalid `together=True` with `facet=True` combinations - add shared x-label and facet-aware title behavior for grouped facets - support `proportion` and `percent` y-axis labels in histogram plotting

- Catch None, "auto", "none" in kwargs['bins'] and route to cal_bin_num to prevent slow seaborn double-binning. - Clarify docstring accordingly - Remove resolved TODO about double-binning logic

- remove redundant `'bins' not in kwargs` branch - tidy related inline comments and docstring wording

- Add validation to ensure group_by is specified when facet=True - Ensure facet and together cannot both be True - Rename target_fig_width/height to facet_fig_width/height for clarity - Update histogram docstring to document facet sizing parameters - Ensure facet_fig_width/height are used in figure sizing

- Reject external ax for grouped-separate and facet modes - Refactor lazy figure creation (only when needed) - Update docstring to document ax parameter constraints - Add tearDown to test cleanup - Add guardrail validation tests

Refactor label validation for facet mode: - Check that individual facet axes have empty labels - Verify figure-level supxlabel/supylabel are set correctly

- Split monolithic facet test into focused test methods - Add smoke test for structure and bar patch presence - Separate titles/label validation into dedicated test - Add density stat label test case - Improve test docstrings and assertions clarity

Refactor facet x-label rotation handling: - Apply rotation to existing label object - Add warning if figure-level label not found - Prevents label override

- Simplify _derive_facet_geometry by delegating numeric parameter normalization to the new helper. - Improves code clarity and reduces duplication. - Adds comprehensive test coverage for geometry derivation.

- template: rotate actual tick labels (anchor/right) - remove _supxlabel rotation branch

- validate facet hint inputs explicitly in histogram() - require facet_fig_width and facet_fig_height to be provided together - simplify _derive_facet_geometry() to consume pre-normalized hints - validate positive figure_width/height in histogram and its template - default automatic facet_ncol to None in histogram and its template - update facet geometry and histogram tests to match strict validation - simplify tests for facet hints to use shared self.adata fixtures

- delete normalize_positive_number from spac.utils - remove unused normalize_positive_number import in visualization - drop obsolete unit tests for removed helper

- pass facet_tick_rotation from template to histogram - extend facet geometry helper with tick-length/rotation-based pressure heuristic - increase facet height and rebalance aspect when long rotated labels are present - keep explicit facet_fig_width/facet_fig_height authoritative - add focused histogram tests for rotation-zero parity and long-label geometry behavior

This fix rejects grouping by annotations with too many categories - Add max_groups guadrail for group_by plotting - Add unittests covering max_groups guardrail

…t layout - replace unconditional facet tight_layout with row-scaled spacing

- narrow `_parse_optional_number` to shared numeric parsing mechanics

- drop template-side allow-list validation for seaborn passthroughs - pass `multiple` only for grouped same-axis overlays - ignore irrelevant `multiple` in grouped non-overlay histogram paths - add regression coverage for grouped separate mode

- add a thin facet smoke test for numeric annotations from adshareata.obs - extract a shared long-label AnnData builder for paired geometry tests - improve d-bin regression setups inline for readability

- reuse grouped histogram-bin tables across together and facet paths - keep categorical shared-slot padding internal to grouped histogram building - restrict Rice-rule auto-bin fallback to numeric data - fold return-data checks into existing histogram tests

zhan4329 · 2026-04-22T01:25:43Z

            "Figure_DPI": 72,
            "Font_Size": 10,
            "Number_of_Bins": 20,
+            "Facet": True,


Add template-level tests for the new validation branches. This fixture still exercises only one successful run even though run_from_json() now owns handled-exception paths for Bins, Max_Groups, Facet, Facet_Ncol, and grouped-facet consistency. Without direct template tests for those inputs, regressions in the JSON contract can slip through even if the lower-level visualization tests still pass. This message is generated by Codex using model GPT-5.4 (xhigh).

Hmmm... @georgezakinih Could you clarify whether I need to add more unittests for templates? From my understanding, the existing template tests only do I/O verification.

zhan4329 · 2026-04-22T01:25:44Z

Additional findings from the local review that do not map cleanly to one changed hunk: this PR adds user-facing histogram and template controls such as facet, facet_ncol, max_groups, and facet sizing hints, but I do not see a corresponding public docs update even though CONTRIBUTING.md says added functionality should update docs. Also, git diff --check upstream/dev...HEAD reports trailing whitespace in touched files, so the diff does not currently satisfy the repo formatting requirement. This message is generated by Codex using model GPT-5.4 (xhigh).

- update documentation for `max_groups` and `facet_ncol` - parse facet figure-size hints only when `facet=True` - add a compact non-facet unittest for ignored hints

zhan4329 · 2026-04-22T06:10:12Z

            "Figure_DPI": 72,
            "Font_Size": 10,
            "Number_of_Bins": 20,
+            "Facet": True,


Hmmm... @georgezakinih Could you clarify whether I need to add more unittests for templates? From my understanding, the existing template tests only do I/O verification.

Addressed comment https://github.com/FNLCR-DMAP/SCSAWorkflow/pull/428/changes#r3121891276 - Ignore facet-only layout hints when facet=False in both histogram() and the histogram template - Tighten template figure-size validation to prevent zero value from bypassing - Update histogram tests for ignored layout hints in non-facet path

Addressed comment https://github.com/FNLCR-DMAP/SCSAWorkflow/pull/428/changes#r3121891313 - Only forward and parse max_groups when group_by is active in the histogram template and histogram() core. - Add focused regression coverage showing non-grouped calls ignore grouped-only max_groups hints.

- tighten _derive_facet_geometry wording - align nested histogram helper docstrings - sync inline comments with current behavior

- normalize `multiple` only when `group_by` and `together` are active

ying39purdue and others added 28 commits March 23, 2026 21:06

histogram rebase to dev

b540bfa

add facet plots on histogram group by

21779cd

add unittest for facet output

09a9012

formatting of facet plot, and unittest

b1693aa

formatting of facet plot, and unittest

a9af695

remove axis rotation and formatting

b5b5699

Unittest addition of element numbers, title, and axis labels check

ff759aa

unittest rebase with dev

7019cae

correct facet function return

44b3566

fix(histogram): enforce shared bins across facets

bc2e95e

fix(histogram): convert numpy array to list for seaborn bins compatib…

674b84e

…ility

fix(histogram): close unused internal figure

6b69eb5

fix(histogram): revert group titles for facet plots without feature s…

248fb45

…pecification

feat(histogram): wire facet layout from template

a2112c2

- add Facet and Facet_Ncol parsing and validation in template - normalize facet_ncol handling in histogram layout kwargs - adjust facet_ncol hints accordingly in histogram docstrings

feat(histogram): add element control and style validation

3c0050b

- expose Element in histogram template inputs - validate multiple, element, and stat values in template - document shrink and alpha keyword behavior in histogram docstring

chore(gitignore): ignore workspace directory

6991def

fix(histogram): normalize bins defaults to Rice-rule estimator

85c570a

- Catch None, "auto", "none" in kwargs['bins'] and route to cal_bin_num to prevent slow seaborn double-binning. - Clarify docstring accordingly - Remove resolved TODO about double-binning logic

refactor(histogram): simplify default bins fallback logic

e5e9082

- remove redundant `'bins' not in kwargs` branch - tidy related inline comments and docstring wording

chore(gitignore): remove workspace entry

b990b8c

test(histogram): update facet label validation assertions

0fc8b32

Refactor label validation for facet mode: - Check that individual facet axes have empty labels - Verify figure-level supxlabel/supylabel are set correctly

zhan4329 changed the title ~~Facet Option on Histogram~~ feat(histogram): add faceted plots with adaptive layout Apr 17, 2026

fix(histogram): preserve facet xlabel and apply rotation

eba316b

Refactor facet x-label rotation handling: - Apply rotation to existing label object - Add warning if figure-level label not found - Prevents label override

zhan4329 added 12 commits April 18, 2026 23:58

refactor(facet): refactor facet geometry using normalize_positive_number

6914244

- Simplify _derive_facet_geometry by delegating numeric parameter normalization to the new helper. - Improves code clarity and reduces duplication. - Adds comprehensive test coverage for geometry derivation.

fix(histogram-template): correct rotated tick label handling in facets

6af0820

- template: rotate actual tick labels (anchor/right) - remove _supxlabel rotation branch

refactor(utils): remove normalize_positive_number helper

5143007

- delete normalize_positive_number from spac.utils - remove unused normalize_positive_number import in visualization - drop obsolete unit tests for removed helper

fix(histogram_template): allow auto figure size in facet mode

367a922

fix(histogram): add max_groups guardrail

886fba2

This fix rejects grouping by annotations with too many categories - Add max_groups guadrail for group_by plotting - Add unittests covering max_groups guardrail

fix(histogram-template): fix figure label overlapping by scaling face…

4062402

…t layout - replace unconditional facet tight_layout with row-scaled spacing

refactor(histogram): simplify optional hint parsing

ef31f50

- narrow `_parse_optional_number` to shared numeric parsing mechanics

test(histogram): improve facet unittests

0198eec

- add a thin facet smoke test for numeric annotations from adshareata.obs - extract a shared long-label AnnData builder for paired geometry tests - improve d-bin regression setups inline for readability