Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 theviolin 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.
Uh oh!
There was an error while loading.Please reload this page.
| elif (isinstance(pos0,np.datetime64) | ||
| andnotisinstance(width0,np.timedelta64)): | ||
| raiseTypeError( | ||
| "np.datetime64 'position' values, require np.timedelta64 'widths'") |
CopilotAIDec 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=widthsifnp.isscalar(widths)elsewidths[0] |
CopilotAIDec 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.
Accessingpositions[0] without checking if positions is non-empty could raise an IndexError. While the code validates thatlen(positions) == N on line 9052, ifN (which equalslen(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=widthsifnp.isscalar(widths)elsewidths[0] |
CopilotAIDec 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 variablewidth0 is assigned fromwidths whennp.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 meanswidth0 will always be assigned fromwidths[0], making the conditional logic incorrect. The check should use the original widths parameter before conversion.
| withpytest.raises(TypeError, | ||
| match="datetime/date 'position' values, require timedelta 'widths'"): |
CopilotAIDec 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)) | ||
| andnotisinstance(width0,datetime.timedelta)): | ||
| raiseTypeError( | ||
| "datetime/date 'position' values, require timedelta 'widths'") |
CopilotAIDec 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)].") |
| deftest_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] | ||
| withpytest.raises(TypeError, | ||
| match="np.datetime64 'position' values, require np.timedelta64 'widths'"): | ||
| ax.violin(violin_plot_stats(),positions=positions,widths=widths) | ||
| deftest_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] | ||
| withpytest.raises(TypeError, | ||
| match="datetime/date 'position' values, require timedelta 'widths'"): | ||
| ax.violin(violin_plot_stats(),positions=positions,widths=widths) | ||
| deftest_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 | ||
| withpytest.raises(TypeError, | ||
| match="datetime/date 'position' values, require timedelta 'widths'"): | ||
| ax.violin(violin_plot_stats(),positions=positions,widths=widths) | ||
| deftest_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) | ||
| deftest_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] | ||
| withpytest.raises(TypeError, | ||
| match="datetime/date 'position' values, require timedelta 'widths'"): | ||
| ax.violin(violin_plot_stats(),positions=positions,widths=widths) |
CopilotAIDec 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) | ||
| deftest_mixed_positions_datetime_and_numeric_behaves(): |
CopilotAIDec 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 nametest_mixed_positions_datetime_and_numeric_behaves is unclear. The suffix "behaves" doesn't indicate what behavior is being tested. Consider renaming totest_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.
| deftest_mixed_positions_datetime_and_numeric_behaves(): | |
| deftest_mixed_positions_datetime_and_numeric_raises(): |
| withpytest.raises(TypeError, | ||
| match="np.datetime64 'position' values, require np.timedelta64 'widths'"): |
CopilotAIDec 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.
| withpytest.raises(TypeError, | ||
| match="datetime/date 'position' values, require timedelta 'widths'"): |
CopilotAIDec 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.
timhoffm commentedDec 18, 2025
From a quick check, AI comments mostly make sense.@hasanrashid please act on them. |
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