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

Add datetime support to _mean() #5368

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

Open
m-ad wants to merge 4 commits into plotly:main
base: main
Choose a base branch
Loading
from m-ad:fix_add_vline_with_annotation_to_datetime_axis

Conversation

@m-ad
Copy link
Contributor

@m-ad m-ad commented Oct 8, 2025

This PR extends the _mean() function in shapeannotation.py to allow for sequences of datetime objects as input. This fixes issue #3065, specifically it enables annotations on hline/vline objects that use datetime axes.

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the code generator and not the generated files.
  • I have added tests or modified existing tests.
  • For a new feature, I have added documentation examples (please see the doc checklist as well).
  • I have added a CHANGELOG entry if changing anything substantial.
  • For a new feature or a change in behavior, I have updated the relevant docstrings in the code.

@gvwilson gvwilson added feature something new P2 considered for next cycle community community contribution labels Oct 9, 2025
Copy link
Contributor

gvwilson commented Oct 9, 2025

@emilykl please review

Copy link
Contributor

emilykl commented Oct 9, 2025
edited
Loading

@m-ad Thank you for the PR!

I'm open to something along these lines as a "patch" fix to enable add_vline() and add_hline() to work with datetime axes, although as @nicolaskruchten mentioned, the "correct" fix would be to refactor the code such that we're not trying to do math with data values.

Ultimately, now that shape labels are natively supported in Plotly.js, I think the longer-term fix here is to refactor add_hline() and add_vline() to add annotation_text as a shape label rather than as a separate annotation; that will allow us to skip the step of computing averages of the data values. I mention it here just in case that's a task you'd be interested in -- I've opened issue #5373 to track.

Otherwise, something like what you've proposed here will work as a temporary fix.

There's a few issues with the logic as written; for example, falling back to returning x[0] is incorrect when x[0] and x[1] are not equal. We should simply raise an error in that case because we have no way of calculating an average.

That means cases like the reproducible example in #3065, where the date value is passed as a string, will require a different approach. I'm not sure exactly what the best approach is; trying to parse every value as a datetime seems a bit clunky, but technically speaking it will produce the correct result. Or maybe we just document this limitation and specify that if you are trying to use add_hline() or add_vline() for a datetime axis you must pass the value as a datetime.

Copy link

@m-ad since we haven't seen any activity on this PR in a while, we may close it in the near future. Let us know if you are still working on it!

Copy link
Contributor Author

m-ad commented Nov 23, 2025
edited
Loading

@m-ad since we haven't seen any activity on this PR in a while, we may close it in the near future. Let us know if you are still working on it!

@robertclaus I am not currently working on it. This is a is a band-aid-style fix and as @emilykl said, the nicer and more "correct" version would be to implement the changes to add_hline() and add_vline() proposed in #5373. Someone appears to be working on that, which would ideally make this PR unnecessary. That said, if you still like to have my band-aid fix around until #5373 is closed, I'd be happy to finalize this PR.

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

Reviewers

@emilykl emilykl Awaiting requested review from emilykl

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

Assignees

@m-ad m-ad

Labels

community community contribution feature something new P2 considered for next cycle

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /