Skip to content

PERF: Override equals in IndexVariable #1256

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

Merged
merged 2 commits into from
Feb 9, 2017
Merged

PERF: Override equals in IndexVariable #1256

merged 2 commits into from
Feb 9, 2017

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Feb 8, 2017

I tried adding some tests but couldn't find a good way of doing that

There is a 20x speed-up though (see issues for prior):

In [51]: indexes = [pd.PeriodIndex(start=str((1776 + i)), freq='A', periods=300) for i in range(50)]
In [53]: das = [xr.DataArray(range(300), coords=[index]) for index in indexes]
 In [54]: %timeit xr.concat(das)



10 loops, best of 3: 70.2 ms per loop

@max-sixty
Copy link
Collaborator Author

max-sixty commented Feb 8, 2017

I don't think the AppVeyor break is related to this

Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

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

This already has pretty good test coverage in test_variable.py, so I think this is fine.

@@ -1247,6 +1247,19 @@ def copy(self, deep=True):
return type(self)(self.dims, self._data, self._attrs,
self._encoding, fastpath=True)

def equals(self, other, equiv=None):
# if equiv is specified, super up
if equiv:
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer equiv is not None, which is slightly more idiomatic for checking default values (I'd rather not rely on the truthiness of arbitrary functions/callables)

@max-sixty max-sixty merged commit ed71fbb into pydata:master Feb 9, 2017
@max-sixty max-sixty deleted the no-unbox-periods branch February 9, 2017 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PeriodIndex causes severe slow down
2 participants