feat: support facet plot in py-maidr using maidr-ts #148
Conversation
…plotlib and seaborn
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for facet plots in py-maidr using the maidr-ts engine and adds several example scripts demonstrating facet plotting with both matplotlib and seaborn. Key changes include:
- New example scripts for facet plotting with matplotlib and seaborn, including combined bar and line plots.
- Updates to extractor mixins and plotting modules to improve shared label extraction and data processing for bar and line plots.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| example/facet-subplots/matplotlib/example_mpl_facet_bar_plot.py | Adds an example of a matplotlib facet bar plot |
| example/facet-subplots/seaborn/example_sns_facet_combined_plot.py | Adds an example of a combined seaborn facet plot for sales and profit trends |
| example/facet-subplots/seaborn/example_sns_facet_bar_plot.py | Implements a simple facet bar plot example using seaborn |
| example/facet-subplots/matplotlib/example_mpl_facet_combined_plot.py | Adds a matplotlib example combining line and bar plots in facets |
| maidr/util/mixin/extractor_mixin.py | Enhances tick label extraction by adding a fallback for shared x-axes |
| maidr/core/plot/barplot.py | Adjusts handling of bar container data and label mismatches |
| maidr/core/plot/maidr_plot.py | Introduces shared xlabel extraction to improve axis labeling consistency |
| maidr/core/plot/lineplot.py | Filters out default line labels for clearer plot annotations |
Comments suppressed due to low confidence (2)
maidr/core/plot/barplot.py:71
- The commented-out length check between plot elements and their labels may mask data mismatches. Consider either removing the commented code entirely if it is no longer needed or refining the check to ensure that the number of patches always aligns with the extracted labels.
if len(plot) != len(level):
maidr/core/plot/lineplot.py:96
- Using the hardcoded string "_child0" as a filter for default labels is brittle. Consider extracting this magic string into a well-named constant to improve maintainability and clarity.
line.get_label() if line.get_label() != "_child0" else ""
|
@dakshpokar Let's merge this PR once you implement seaborn support and its example. |
|
Sure Professor @jooyoungseo! |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for facet plots in py-maidr using maidr-ts and adds several new methods to handle shared axis label extraction across different plot types. Key changes include:
- Adding a static method to extract shared x tick labels in mixin classes.
- Creating a new method to extract a shared xlabel and updating axis data extraction in maidr_plot.
- Updating label extraction in the lineplot and barplot to better handle special cases, and adding several new example scripts for seaborn and matplotlib facet plots.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| maidr/util/mixin/extractor_mixin.py | Added fallback to extract shared xtick labels when level is empty. |
| maidr/core/plot/maidr_plot.py | Introduced extract_shared_xlabel and updated axis data extraction with a default "X" label. |
| maidr/core/plot/lineplot.py | Updated label extraction to ignore placeholder labels in line plots. |
| maidr/core/plot/barplot.py | Modified level extraction to assign empty strings when no labels are found and commented out a length mismatch check. |
| example/facet-subplots/seaborn/* | Added new example scripts demonstrating facet plot implementations using seaborn. |
| example/facet-subplots/matplotlib/* | Added new example scripts demonstrating facet plot implementations using matplotlib. |
| def _extract_axes_data(self) -> dict: | ||
| """Extract the plot's axes data""" | ||
| return {MaidrKey.X: self.ax.get_xlabel(), MaidrKey.Y: self.ax.get_ylabel()} | ||
| x_labels = self.ax.get_xlabel() | ||
| if not x_labels: | ||
| x_labels = self.extract_shared_xlabel(self.ax) | ||
| if not x_labels: | ||
| x_labels = "X" |
There was a problem hiding this comment.
Defaulting the xlabel to 'X' when no label is found might lead to misleading or unintended axis labeling. It may be beneficial to parameterize this default or add documentation explaining the rationale.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
"X" is just the “last‐resort” label and only shows up if no X‑axis label can be found anywhere in your figure or shared group.
|
Professor @jooyoungseo, this PR is ready for merge! |
Description
Note: This doesn't support Seaborn's FacetGrid construction of the plot yet.
Type of Change
Checklist