-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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.
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
base: main
Are you sure you want to change the base?
Conversation
| 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 | ||
| }] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
There was a problem hiding this comment.
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).
There was a problem hiding this 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) |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| ax.violin(violin_plot_stats(), positions=positions, widths=widths) | |
| ax.violin(violin_plot_stats(), positions=positions, widths=widths) | |
| plt.close(fig) |
| elif (isinstance(pos0, np.datetime64) | ||
| and not isinstance(width0, np.timedelta64)): | ||
| raise TypeError( | ||
| "np.datetime64 'position' values, require np.timedelta64 'widths'") |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| "np.datetime64 'position' values, require np.timedelta64 'widths'") | |
| "np.datetime64 'position' values require np.timedelta64 'widths'") |
| pos0 = positions[0] | ||
| width0 = widths if np.isscalar(widths) else widths[0] |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| # 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] |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| with pytest.raises(TypeError, | ||
| match="datetime/date 'position' values, require timedelta 'widths'"): |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| if (isinstance(pos0, (datetime.datetime, datetime.date)) | ||
| and not isinstance(width0, datetime.timedelta)): | ||
| raise TypeError( | ||
| "datetime/date 'position' values, require timedelta 'widths'") |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| "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)].") |
| 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) |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| ax.violin(violin_plot_stats(), positions=positions, widths=widths) | ||
|
|
||
|
|
||
| def test_mixed_positions_datetime_and_numeric_behaves(): |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| def test_mixed_positions_datetime_and_numeric_behaves(): | |
| def test_mixed_positions_datetime_and_numeric_raises(): |
| with pytest.raises(TypeError, | ||
| match="np.datetime64 'position' values, require np.timedelta64 'widths'"): |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
| with pytest.raises(TypeError, | ||
| match="datetime/date 'position' values, require timedelta 'widths'"): |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
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.
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