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

Revert "REF: back IntervalArray by a single ndarray (#37047)" #38024

Merged
merged 1 commit into from
Nov 24, 2020
Merged

Revert "REF: back IntervalArray by a single ndarray (#37047)" #38024

merged 1 commit into from
Nov 24, 2020

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Nov 23, 2020 •
edited
Loading

This reverts commit 9cb3723.

See discussion in #37047

@jbrockmendel can you give this a check? (there were already several conflicts)

@jorisvandenbossche jorisvandenbossche added this to the 1.2 milestone Nov 23, 2020
@jorisvandenbossche jorisvandenbossche added Interval Interval data type Refactor Internal refactoring of code labels Nov 23, 2020
@jbrockmendel
Copy link
Member

I'll take a look, tentatively looks OK.

By funny coincidence, I just got the first of the promised perf improvements working.

jreback
jreback requested changes Nov 24, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

ok with the change back.

pandas/core/arrays/interval.py
left = left.astype(dtype.subtype)
right = right.astype(dtype.subtype)

# coerce dtypes to match if needed
Copy link
Contributor

Choose a reason for hiding this comment

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

can you factor this out into a function (maybe_cast_inputs), this is crazy otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

This is how it was before (I am just reverting / fixing conflicts). I prefer to keep this PR a clean revert, and do any other refactor as follow-up

Copy link
Contributor

Choose a reason for hiding this comment

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

i c ok.

@jreback
Copy link
Contributor

jreback commented Nov 24, 2020

@jbrockmendel ok here?

@jbrockmendel
Copy link
Member

looks fine

@jreback jreback merged commit b19e47f into pandas-dev:master Nov 24, 2020
@jorisvandenbossche jorisvandenbossche deleted the revert-intervalarray-backing branch November 24, 2020 19:13
@jorisvandenbossche
Copy link
Member Author

I just got the first of the promised perf improvements working.

Do you have a branch for it that you can push? I am curious to see whether the principles of the speed-up also couldn't be applied to the "2 1D-arrays" model

@jbrockmendel
Copy link
Member

Do you have a branch for it that you can push? I am curious to see whether the principles of the speed-up also couldn't be applied to the "2 1D-arrays" model

shortly, i hope. The approach is to do self._combined.view("complex128") and then do set operations on that to avoid casting to object.

@jorisvandenbossche
Copy link
Member Author

I can imagine that an additional copy to create the combined array could still be worth it compared to casting to object

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

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Interval Interval data type Refactor Internal refactoring of code
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.
3 participants
@jorisvandenbossche @jbrockmendel @jreback