Skip to content

Conversation

Dando18
Copy link
Collaborator

@Dando18 Dando18 commented Oct 20, 2025

No description provided.

@Dando18 Dando18 requested a review from Copilot October 20, 2025 22:47
@Dando18 Dando18 self-assigned this Oct 20, 2025
@Dando18 Dando18 added the enhancement New feature or request label Oct 20, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new BoxPlot class to the pssgplot library, providing functionality for creating box plot visualizations with customization options including hatching patterns, labels, legends, and axis formatting. The implementation includes an animation method stub that is currently not implemented.

Key Changes:

  • New BoxPlot class with comprehensive plotting options and seaborn integration
  • Support for hatching patterns, error bars, labels, and legend customization
  • Animation method placeholder (not yet implemented)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
pssgplot/boxplot.py Introduces the complete BoxPlot class with plotting and animation methods
pssgplot/init.py Exports the new BoxPlot class for public API access

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +107 to +119
for p in self.ax.patches:
if p.get_width() <= 0:
continue
self.ax.annotate(
label_fmt.format(p.get_height()),
(p.get_x() + p.get_width() / 2., p.get_height()),
ha='center',
va='center',
xytext=(0, 10),
textcoords='offset points',
fontsize=label_fontsize
)

Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label positioning logic uses get_width() and get_height() which are appropriate for bar plots, but box plots use different patch geometries. For box plots, you should use the patch's bounding box or specific box plot coordinates to position labels correctly.

Suggested change
for p in self.ax.patches:
if p.get_width() <= 0:
continue
self.ax.annotate(
label_fmt.format(p.get_height()),
(p.get_x() + p.get_width() / 2., p.get_height()),
ha='center',
va='center',
xytext=(0, 10),
textcoords='offset points',
fontsize=label_fontsize
)
# Find median lines and annotate at their positions
medians = [line for line in self.ax.lines if line.get_linestyle() == '-']
for median in medians:
xdata = median.get_xdata()
ydata = median.get_ydata()
# The median line is horizontal, so ydata[0] == ydata[1]
x = (xdata[0] + xdata[1]) / 2.
y = ydata[0]
self.ax.annotate(
label_fmt.format(y),
(x, y),
ha='center',
va='center',
xytext=(0, 10),
textcoords='offset points',
fontsize=label_fontsize
)

Copilot uses AI. Check for mistakes.

Comment on lines +157 to +187

if not self.ax or not self.fig:
raise ValueError("Plot must be created before animating.")

self.ax.set_visible(False)

if by == 'column':
frames = len(self.data[self.x].unique())
group_size = 1
elif by == 'hue' and 'hue' in self.kwargs:
frames = len(self.data[self.kwargs['hue']].unique())
group_size = len(self.data[self.x].unique())
else:
raise ValueError("Invalid value for 'by'. Must be 'column' or 'hue' with 'hue' in kwargs.")

# check if save_dir exists; if not, create it; if it does, error if it's not empty
if not os.path.exists(save_dir):
os.makedirs(save_dir)
#elif os.listdir(save_dir):
# raise ValueError(f"Directory {save_dir} is not empty.")

# save each frame as frame_format; and also save a gif
for i in range(frames):
self.ax.set_visible(True)
for j, bar in enumerate(self.ax.patches):
if j+1 <= (i+1)*group_size:
#if j <= i:
bar.set_visible(True)
else:
bar.set_visible(False)
self.fig.savefig(os.path.join(save_dir, f"frame_{i}.{frame_format}"), **kwargs)
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is unreachable because the function raises NotImplementedError at line 156 before this validation. Either remove the unreachable code or move the NotImplementedError to after the validation checks.

Suggested change
if not self.ax or not self.fig:
raise ValueError("Plot must be created before animating.")
self.ax.set_visible(False)
if by == 'column':
frames = len(self.data[self.x].unique())
group_size = 1
elif by == 'hue' and 'hue' in self.kwargs:
frames = len(self.data[self.kwargs['hue']].unique())
group_size = len(self.data[self.x].unique())
else:
raise ValueError("Invalid value for 'by'. Must be 'column' or 'hue' with 'hue' in kwargs.")
# check if save_dir exists; if not, create it; if it does, error if it's not empty
if not os.path.exists(save_dir):
os.makedirs(save_dir)
#elif os.listdir(save_dir):
# raise ValueError(f"Directory {save_dir} is not empty.")
# save each frame as frame_format; and also save a gif
for i in range(frames):
self.ax.set_visible(True)
for j, bar in enumerate(self.ax.patches):
if j+1 <= (i+1)*group_size:
#if j <= i:
bar.set_visible(True)
else:
bar.set_visible(False)
self.fig.savefig(os.path.join(save_dir, f"frame_{i}.{frame_format}"), **kwargs)

Copilot uses AI. Check for mistakes.

Comment on lines +134 to +135
if 'hue' in kwargs:
for i, p in enumerate(self.ax.get_legend().get_patches()):
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing self.ax.get_legend() without checking if a legend exists will raise an AttributeError if no legend was created. This code path is only reached when hatch=True, but the legend is only created when legend=True (line 98). Add a check: if 'hue' in kwargs and self.ax.get_legend() is not None:

Copilot uses AI. Check for mistakes.

@Dando18 Dando18 requested a review from bhatele October 20, 2025 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant