Improving error message for width and position type mismatch in violinplot #30752
Conversation
There was a problem hiding this comment.
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.
|
From a quick check, AI comments mostly make sense. @hasanrashid please act on them. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…alidation - Improve pytest match strings with proper line continuation formatting - Enhanced error messages provide clear examples for correct usage
lib/matplotlib/axes/_axes.py
Outdated
| return {key: [] for key in [ | ||
| 'bodies', 'cmeans', 'cmins', 'cmaxes', 'cbars', | ||
| 'cmedians', 'cquantiles']} |
There was a problem hiding this comment.
This is not correct. So far:
In [3]: ax.violin([])
Out[3]:
{'bodies': [],
'cmaxes': <matplotlib.collections.LineCollection at 0x787bf23cb080>,
'cmins': <matplotlib.collections.LineCollection at 0x787be8142ea0>,
'cbars': <matplotlib.collections.LineCollection at 0x787be821f9e0>}While early returns are usually good, I wouldn't do it in this case as you'd have to exactly figure out how to construct the complex return type. Just let it fall through as before
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
lib/matplotlib/axes/_axes.py
Outdated
| elif len(positions) != N: | ||
| raise ValueError(datashape_message.format("positions")) | ||
|
|
||
| pre_conversion_widths = widths |
There was a problem hiding this comment.
| pre_conversion_widths = widths |
is now unused
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
2 participants
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