Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pandas 2 compability: Fix a bug regarding timedelta64[ms] representation #4102 #4103

Merged
merged 1 commit into from Apr 5, 2023
Merged

Pandas 2 compability: Fix a bug regarding timedelta64[ms] representation #4102 #4103

merged 1 commit into from Apr 5, 2023

Conversation

kaibir
Copy link
Contributor

@kaibir kaibir commented Mar 14, 2023 •
edited

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 codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

Close #4102

@aberres
Copy link

aberres commented Mar 15, 2023

@nicolaskruchten As Pandas 2 is already in the RC phase with a final release expected in the next weeks, it would be nice to see this in the next release. The change is backwards compatible.

@alexcjohnson
Copy link
Contributor

Thanks @kaibir - and @aberres I agree, would be great to get this in ASAP, and we'll be working to make a release in the next day or two. What I'd love to do before we merge this though is add a test job (or two I guess, get both _core and _optional) that uses pandas 2 on Py3.11, and see if it surfaces any other issues. I think would just need to be two new jobs in https://github.com/plotly/plotly.py/blob/master/.circleci/config.yml plus two new requirements files in https://github.com/plotly/plotly.py/tree/master/packages/python/plotly/test_requirements - if either of you would be able to give that a go it would be a big help, otherwise we can try to get to it before we need to make the next release.

@aberres
Copy link

aberres commented Mar 16, 2023

@alexcjohnson We can give it a try.

As I see things, the currently used images were deprecated, and there is now cimg/python. My feeling is this switch should be made in a different PR.

As Pandas happily works with 3.9 (the newest Python version checked right now), maybe we should start adding another _optional with Pandas 2 based on python_39_optional for starters?

@kaibir kaibir force-pushed the master branch 3 times, most recently from cfd8d09 to ca4be23 Compare March 16, 2023 11:03
aberres
aberres reviewed Mar 16, 2023
View reviewed changes
packages/python/plotly/_plotly_utils/tests/validators/test_pandas_series_input.py
"float16",
# "float16",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson Python 2.0 forbids creating float16 indxes:

image

The easiest thing to do would be to drop this line here.

aberres
aberres reviewed Mar 17, 2023
View reviewed changes
packages/python/plotly/test_requirements/requirements_39_pandas_2_optional.txt Outdated
@@ -0,0 +1,21 @@
requests==2.25.1
tenacity==6.2.0
pandas==2.0.0rc0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaibir We could bump this to the RC1 now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have updated it. @aberres

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumped to 2.0.0 as there is the new pandas release since yesterday.

@kaibir kaibir force-pushed the master branch 2 times, most recently from beb319a to 129c717 Compare March 20, 2023 10:59
@aberres aberres mentioned this pull request Apr 4, 2023
Closed
@kaibir kaibir changed the title Fix a bug regarding timedelta64[ms] representation #4102 Pandas 2 compability: Fix a bug regarding timedelta64[ms] representation #4102 Apr 4, 2023
@aberres
Copy link

aberres commented Apr 4, 2023

@alexcjohnson Friendly ping. Anything we can do to get this 🚀?

Pandas 2 is out since yesterday.

alexcjohnson
alexcjohnson reviewed Apr 4, 2023
View reviewed changes
packages/python/plotly/plotly/__init__.py
@@ -102,7 +102,8 @@ def plot(data_frame, kind, **kwargs):
if kind == "line":
return line(data_frame, **kwargs)
if kind == "area":
return area(data_frame, **kwargs)
new_kwargs = {k: kwargs[k] for k in kwargs if k not in ["stacked"]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a question for @nicolaskruchten as much as for you @kaibir - since there are already other examples of these exclusions in other kinds, but how do we know what needs to be excluded?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It raised in a test 🙈

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah in principle the Pandas team isn't meant to add kwargs or mess with that API much any more but if/when they do (by e.g. adding upstream kwargs) then our tests will fail.

alexcjohnson
alexcjohnson approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 This looks great @kaibir @aberres - thanks for adding the new test run! And apologies for the delay, I was OOO for the last 2 weeks.

My comment is more for my own info than blocking.

@alexcjohnson alexcjohnson merged commit cb27da7 into plotly:master Apr 5, 2023
4 checks passed
@kaibir
Copy link
Contributor Author

kaibir commented Apr 6, 2023

Thanks for the quick release @LiamConnors @alexcjohnson.

I think #4145 should also be solved by this.

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

@nicolaskruchten nicolaskruchten nicolaskruchten left review comments

@aberres aberres aberres left review comments

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Creating a timeline does not work with [new] pandas 2.0.0
4 participants
@kaibir @aberres @alexcjohnson @nicolaskruchten