🌐 AI搜索 & 代理 主页
Skip to content

Improving error message for width and position type mismatch in violinplot #30752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hasanrashid wants to merge 7 commits into
base: main
Choose a base branch
from
+89 −0
Open

Improving error message for width and position type mismatch in violinplot #30752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7b44fcb
Improving error message for width and position type mismatch in violi…
hasanrashid Nov 16, 2025
4d9c8cd
Improving error message for width and position type mismatch in violi…
hasanrashid Nov 16, 2025
555f4d5
Merge branch 'enh-30417-v1' of github.com:hasanrashid/matplotlib into…
hasanrashid Nov 16, 2025
c40820b
Fix violin plot statistics in test data
hasanrashid Nov 29, 2025
2cb8aba
Trigger CI pipeline
hasanrashid Dec 7, 2025
8ff4f12
Trigger CI pipeline
hasanrashid Dec 7, 2025
b0862b8
Merge branch 'enh-30417-v1' of github.com:hasanrashid/matplotlib into…
hasanrashid Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lib/matplotlib/axes/_axes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import itertools
import logging
import math
import datetime
from numbers import Integral, Number, Real

import re
Expand Down Expand Up @@ -9057,6 +9058,19 @@ def violin(self, vpstats, positions=None, vert=None,
elif len(widths) != N:
raise ValueError(datashape_message.format("widths"))

# For usability / better error message:
# Validate that datetime-like positions have timedelta-like widths.
# Checking only the first element is good enough for standard misuse cases
pos0 = positions[0]
width0 = widths if np.isscalar(widths) else widths[0]
Comment on lines +9064 to +9065
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

Accessing positions[0] without checking if positions is non-empty could raise an IndexError. While the code validates that len(positions) == N on line 9052, if N (which equals len(vpstats)) is 0, positions could be an empty list. Consider adding a check for empty positions/vpstats or documenting that vpstats must be non-empty.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The variable width0 is assigned from widths when np.isscalar(widths) is True, but at this point in the code, widths has already been converted to a list on line 9057 when it was a scalar. This means width0 will always be assigned from widths[0], making the conditional logic incorrect. The check should use the original widths parameter before conversion.

Copilot uses AI. Check for mistakes.
if (isinstance(pos0, (datetime.datetime, datetime.date))
and not isinstance(width0, datetime.timedelta)):
raise TypeError(
"datetime/date 'position' values, require timedelta 'widths'")
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The error message "datetime/date 'position' values, require timedelta 'widths'" has incorrect grammar. The comma after "values" creates a comma splice. The message should either be: "datetime/date 'position' values require timedelta 'widths'" (remove comma) or "For datetime/date 'position' values, timedelta 'widths' are required" (restructure). Additionally, the message would be more helpful if it included an example as mentioned in the PR description.

Suggested change
"datetime/date 'position' values, require timedelta 'widths'")
"datetime/date 'position' values require timedelta 'widths'. "
"For example, use positions=[datetime.date(2024, 1, 1)] and widths=[datetime.timedelta(days=1)].")

Copilot uses AI. Check for mistakes.
elif (isinstance(pos0, np.datetime64)
and not isinstance(width0, np.timedelta64)):
raise TypeError(
"np.datetime64 'position' values, require np.timedelta64 'widths'")
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The error message "np.datetime64 'position' values, require np.timedelta64 'widths'" has the same grammatical issue as the previous error message. The comma after "values" creates a comma splice. Consider removing the comma or restructuring the message for better clarity and grammar.

Suggested change
"np.datetime64 'position' values, require np.timedelta64 'widths'")
"np.datetime64 'position' values require np.timedelta64 'widths'")

Copilot uses AI. Check for mistakes.
# Validate side
_api.check_in_list(["both", "low", "high"], side=side)

Expand Down
75 changes: 75 additions & 0 deletions lib/matplotlib/tests/test_axes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4121,6 +4121,81 @@ def test_violinplot_sides():
showextrema=True, showmedians=True, side=side)


def violin_plot_stats():
datetimes = [
datetime.datetime(2023, 2, 10),
datetime.datetime(2023, 5, 18),
datetime.datetime(2023, 6, 6)
]
return [{
'coords': datetimes,
'vals': [1.2, 2.8, 1.5],
'mean': 1.84,
'median': 1.5,
'min': 1.2,
'max': 2.8,
'quantiles': [1.2, 1.5, 2.8]
}, {
'coords': datetimes,
'vals': [0.8, 1.1, 0.9],
'mean': 0.94,
'median': 0.9,
'min': 0.8,
'max': 1.1,
'quantiles': [0.8, 0.9, 1.1]
}]


def test_datetime_positions_with_datetime64():
"""Test that datetime positions with float widths raise TypeError."""
fig, ax = plt.subplots()
positions = [np.datetime64('2020-01-01'), np.datetime64('2021-01-01')]
widths = [0.5, 1.0]
with pytest.raises(TypeError,
match="np.datetime64 'position' values, require np.timedelta64 'widths'"):
Comment on lines +4154 to +4155
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The match string for pytest.raises is split across two lines without proper line continuation formatting. This could lead to issues if formatting changes or the line is refactored. Consider using proper line continuation with parentheses or joining into a single line for better maintainability and consistency with pytest best practices.

Copilot uses AI. Check for mistakes.
ax.violin(violin_plot_stats(), positions=positions, widths=widths)


def test_datetime_positions_with_float_widths_raises():
"""Test that datetime positions with float widths raise TypeError."""
fig, ax = plt.subplots()
positions = [datetime.datetime(2020, 1, 1), datetime.datetime(2021, 1, 1)]
widths = [0.5, 1.0]
with pytest.raises(TypeError,
match="datetime/date 'position' values, require timedelta 'widths'"):
Comment on lines +4164 to +4165
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The match string for pytest.raises is split across two lines without proper line continuation formatting. This could lead to issues if formatting changes or the line is refactored. Consider using proper line continuation with parentheses or joining into a single line for better maintainability and consistency with pytest best practices.

Copilot uses AI. Check for mistakes.
ax.violin(violin_plot_stats(), positions=positions, widths=widths)


def test_datetime_positions_with_scalar_float_width_raises():
"""Test that datetime positions with scalar float width raise TypeError."""
fig, ax = plt.subplots()
positions = [datetime.datetime(2020, 1, 1), datetime.datetime(2021, 1, 1)]
widths = 0.75
with pytest.raises(TypeError,
match="datetime/date 'position' values, require timedelta 'widths'"):
Comment on lines +4174 to +4175
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The match string for pytest.raises is split across two lines without proper line continuation formatting. This could lead to issues if formatting changes or the line is refactored. Consider using proper line continuation with parentheses or joining into a single line for better maintainability and consistency with pytest best practices.

Copilot uses AI. Check for mistakes.
ax.violin(violin_plot_stats(), positions=positions, widths=widths)


def test_numeric_positions_with_float_widths_ok():
"""Test that numeric positions with float widths work."""
fig, ax = plt.subplots()
positions = [1.0, 2.0]
widths = [0.5, 1.0]
ax.violin(violin_plot_stats(), positions=positions, widths=widths)
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

This test creates a figure but doesn't close it. While matplotlib typically handles this, it's better practice to either use a pytest fixture that manages figure lifecycle, or explicitly close the figure to avoid resource leaks in the test suite. Consider adding plt.close(fig) after the violin call or using a context manager.

Suggested change
ax.violin(violin_plot_stats(), positions=positions, widths=widths)
ax.violin(violin_plot_stats(), positions=positions, widths=widths)
plt.close(fig)

Copilot uses AI. Check for mistakes.


def test_mixed_positions_datetime_and_numeric_behaves():
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The test function name test_mixed_positions_datetime_and_numeric_behaves is unclear. The suffix "behaves" doesn't indicate what behavior is being tested. Consider renaming to test_mixed_positions_datetime_and_numeric_raises to be consistent with the other test names in this group and clearly indicate that this test expects an error to be raised.

Suggested change
def test_mixed_positions_datetime_and_numeric_behaves():
def test_mixed_positions_datetime_and_numeric_raises():

Copilot uses AI. Check for mistakes.
"""Test that mixed datetime and numeric positions
with float widths raise TypeError.
"""
fig, ax = plt.subplots()
positions = [datetime.datetime(2020, 1, 1), 2.0]
widths = [0.5, 1.0]
with pytest.raises(TypeError,
match="datetime/date 'position' values, require timedelta 'widths'"):
Comment on lines +4194 to +4195
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The match string for pytest.raises is split across two lines without proper line continuation formatting. This could lead to issues if formatting changes or the line is refactored. Consider using proper line continuation with parentheses or joining into a single line for better maintainability and consistency with pytest best practices.

Copilot uses AI. Check for mistakes.
ax.violin(violin_plot_stats(), positions=positions, widths=widths)
Comment on lines +4149 to +4196
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

While these tests verify that incorrect combinations of datetime positions with float widths raise errors, there's no test for the successful case where datetime positions are paired with timedelta widths (or np.datetime64 with np.timedelta64). Add positive test cases to ensure that the correct usage actually works and doesn't raise an error, similar to test_numeric_positions_with_float_widths_ok.

Copilot uses AI. Check for mistakes.


def test_violinplot_bad_positions():
ax = plt.axes()
# First 9 digits of frac(sqrt(47))
Expand Down
Loading