Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
hasanrashid wants to merge7 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromhasanrashid:enh-30417-v1

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

CopilotAI review requested due to automatic review settingsDecember 15, 2025 00:15
Copy link

CopilotAI 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 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 tolib/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.

FileDescription
lib/matplotlib/axes/_axes.pyAdded datetime import and validation logic to check position/width type compatibility before arithmetic operations
lib/matplotlib/tests/test_axes.pyAdded 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.

elif (isinstance(pos0,np.datetime64)
andnotisinstance(width0,np.timedelta64)):
raiseTypeError(
"np.datetime64 'position' values, require np.timedelta64 'widths'")

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=widthsifnp.isscalar(widths)elsewidths[0]

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.

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=widthsifnp.isscalar(widths)elsewidths[0]

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.

Copilot uses AI. Check for mistakes.
Comment on lines +4174 to +4175
withpytest.raises(TypeError,
match="datetime/date 'position' values, require timedelta 'widths'"):

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))
andnotisinstance(width0,datetime.timedelta)):
raiseTypeError(
"datetime/date 'position' values, require timedelta 'widths'")

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
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)

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)


deftest_mixed_positions_datetime_and_numeric_behaves():

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.

Suggested change
deftest_mixed_positions_datetime_and_numeric_behaves():
deftest_mixed_positions_datetime_and_numeric_raises():

Copilot uses AI. Check for mistakes.
Comment on lines +4154 to +4155
withpytest.raises(TypeError,
match="np.datetime64 'position' values, require np.timedelta64 'widths'"):

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
withpytest.raises(TypeError,
match="datetime/date 'position' values, require timedelta 'widths'"):

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.
@timhoffm
Copy link
Member

From a quick check, AI comments mostly make sense.@hasanrashid please act on them.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@timhoffmtimhoffmtimhoffm left review comments

Copilot code reviewCopilotCopilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hasanrashid@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp