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

Conversation

@hasanrashid
Copy link
Contributor

This PR intends to close [ENH]: Support using datetimes as positions argument to violin(...) #30417

It should be possible to set the position of a violin plot to be a datetime. Currently, an attempt to do this results in this error message: TypeError: unsupported operand type(s) for +: 'float' and 'datetime.datetime'

The error stems from trying to perform operations between float and datetime if datetime was provided as position arguments.

The proposed solution improves the error message to be:

"If positions are datetime/date values, pass widths as datetime.timedelta (e.g., datetime.timedelta(days=10)) or numpy.timedelta64.

unit tests are in tests\test_violinplot_datetime.py

I had opened another PR #30545, but messed up the commits while making changes. I am making this one after reading the suggestion here: #30508 (comment) by @rcomer . This change updates the error message instead of converting the position and width

Comment on lines 4124 to 4146
def violin_plot_stats():
datetimes = [
datetime.datetime(2023, 2, 10),
datetime.datetime(2023, 5, 18),
datetime.datetime(2023, 6, 6)
]
return [{
'coords': datetimes,
'vals': [0.1, 0.5, 0.2],
'mean': datetimes[1],
'median': datetimes[1],
'min': datetimes[0],
'max': datetimes[-1],
'quantiles': datetimes
}, {
'coords': datetimes,
'vals': [0.2, 0.3, 0.4],
'mean': datetimes[2],
'median': datetimes[2],
'min': datetimes[0],
'max': datetimes[-1],
'quantiles': datetimes
}]
Copy link
Member

Choose a reason for hiding this comment

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

This PR handles date aspects on x-axis-related quantities. The stats refer to the y-axis and their unit does not matter in this PR. For simplicity and clarity, let's not use dates for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timhoffm

return [{
       'coords': datetimes,
       'vals': [0.1, 0.5, 0.2],
       'mean': datetimes[1],
       'median': datetimes[1],
       'min': datetimes[0],
       'max': datetimes[-1],
       'quantiles': datetimes

The quantities are the y-axis values- that, and the mean, median, min and max values for the quantities spread is what you are asking me to change? Just wanted to confirm

Something like this:

   'coords': datetimes,
       'vals': [0.1, 0.5, 0.2],
       'mean': 0.5,
       'median': 0.5,
       'min': 0.1,
       'max': 0.2,
       'quantiles': [0.1, 0.5, 0.2]

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but please do not use obviously nonsensical data (like max < mean).

Copilot AI review requested due to automatic review settings December 15, 2025 00:15
Copy link

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 improves error messaging in the violin method to provide clearer feedback when datetime-like position arguments are used with incompatible float width values. The changes add validation that checks type compatibility between positions and widths, raising a TypeError with a helpful message before the cryptic type operation error would occur during arithmetic operations.

Key changes:

  • Added datetime import to lib/matplotlib/axes/_axes.py
  • Implemented validation logic to detect datetime/date or np.datetime64 positions paired with non-timedelta widths
  • Added comprehensive test coverage for the error cases and normal numeric position usage

Reviewed changes

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

File Description
lib/matplotlib/axes/_axes.py Added datetime import and validation logic to check position/width type compatibility before arithmetic operations
lib/matplotlib/tests/test_axes.py Added test helper function and 5 test cases covering datetime64, datetime, scalar widths, mixed positions, and numeric positions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.
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.
Comment on lines +9064 to +9065
pos0 = positions[0]
width0 = widths if np.isscalar(widths) else widths[0]
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.
# 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]
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.
Comment on lines +4174 to +4175
with pytest.raises(TypeError,
match="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 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.
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.
Comment on lines +4149 to +4196
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'"):
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'"):
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'"):
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)


def test_mixed_positions_datetime_and_numeric_behaves():
"""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'"):
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.

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.
ax.violin(violin_plot_stats(), positions=positions, widths=widths)


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.
Comment on lines +4154 to +4155
with pytest.raises(TypeError,
match="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 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.
Comment on lines +4164 to +4165
with pytest.raises(TypeError,
match="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 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants