feat: Implement violin plot support with dual-layer registration #258
+2,977
−1,008
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tion to handle None values and ensure proper data extraction
…hPlot with violin fill support
…g box plot data and violin polygons with tick positions
…nctions to clean up code
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for violin plots with embedded box plot statistics in the MAIDR system. It enables both KDE (violin shape) and box plot layers to be extracted from seaborn violin plots, making them accessible for visualization.
- Implements
ViolinBoxPlotclass for rendering box plot statistics within violin plots - Adds automatic box plot statistics calculation from raw violin data
- Registers both SMOOTH (KDE) and BOX layers for violin plots with proper element tracking
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| maidr/patch/violinplot.py | New file implementing violin plot patching with box plot extraction, element creation, and KDE registration |
| maidr/patch/init.py | Adds violinplot module to patch imports |
| maidr/core/plot/regplot.py | Extends SmoothPlot to support violin density calculations and category naming |
Comments suppressed due to low confidence (4)
maidr/patch/violinplot.py:241
- Variable _whisker_width is not used.
_whisker_width = 0.1 # kept for potential future customization
maidr/patch/violinplot.py:275
- Variable _whisker_height is not used.
_whisker_height = 0.1 # kept for potential future customization
maidr/patch/violinplot.py:478
- Variable _sorted_tick_positions is not used.
_sorted_tick_positions = [pos for pos, _ in position_group_pairs]
maidr/patch/violinplot.py:483
- Variable _group_positions is not used.
_group_positions = {group: idx for idx, group in enumerate(unique_groups)}
…cumentation and examples for better usability
Bhoomika2905
changed the title
Collaborator
|
@Bhoomika2905 , Could you please address these issues? Layer separation violation ViolinBoxPlot is in maidr/patch/violinplot.py but should be in maidr/core/plot/. Patches should only contain wrapper functions Factory pattern bypass: Code directly calls FigureManager._get_maidr() and manually appends plots and should use FigureManager.create_maidr() like all other patches Missing factory registration: ViolinBoxPlot is not registered in MaidrPlotFactory Looking at create_violin_box_elements() function (lines 632-638): The boxplot lines/rectangles are not part of the original seaborn.violinplot() output - they're added afterward. It directly manipulates the axes. We ideally shouldn't add these, rather extract the selectors from already rendered boxplot. The violin patch should: Detect both BOX and SMOOTH layers (already does this) Use dedicated classes for each: ViolinBoxPlot for box statistics |
Collaborator
@Bhoomika2905 , I think there is an issue with payload construction....I just see only box & smooth layer, and that is reason why there might be arch violations, You might have to introduce a separate violin class for processing these together. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds comprehensive support for violin plots in py-maidr, enabling automatic registration of violin plots created with seaborn for MAIDR's accessibility pipeline. Violin plots are now registered with both BOX and SMOOTH (KDE) layers, providing full accessibility features including highlighting, navigation, and data extraction.
Type of Change
Checklist
Pull Request
Description
seaborn.violinplot()are automatically registered with MAIDR when the module is importedChanges Made
New Files
maidr/patch/violinplot.py: Complete implementation of violin plot supportViolinBoxPlotclass: Custom plot class for violin/box plotscalculate_box_stats_from_violin_data(): Calculates box plot statistics from violin datacreate_violin_box_elements(): Creates box plot elements (lines, rectangles) for highlightingcreate_violin_box_data(): Creates MAIDR-compatible box plot data structuresns_violin(): Wrapper function that patchesseaborn.violinplot()Modified Files
maidr/core/plot/regplot.py: EnhancedSmoothPlotclass to support violin plotsviolin_fillparameter for category/group namesScreenshots (if applicable)
Checklist
ManualTestingProcess.md, and all tests related to this pull request pass.Additional Notes