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

Simplified DynamicMap reindex as it is handled automatically #1267

Merged
merged 2 commits into from Apr 11, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Apr 11, 2017

The new Callable inspection in _execute_callback now automatically remaps key dimensions onto the callback so wrapping the callable is no longer needed.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 11, 2017

I also added made sure that any clone of a DynamicMap references the original DynamicMap in its Callable.inputs, ensuring that the original source is inherited.

@philippjfr philippjfr requested a review from jlstevens Apr 11, 2017

# Ensure the clone references this object to ensure
# stream sources are inherited
if clone.callback is self.callback:
clone.callback = self.callback.clone()

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2017

Member

I don't quite get this line. Why would self.callback have a clone method? Isn't that a Callable around the callback?

This comment has been minimized.

@philippjfr

philippjfr Apr 11, 2017

Member

Yes, see above I gave it a clone method. Can do it manually here I suppose and remove it again. Up to you.

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2017

Member

It is at the top of the diff and I must have just skimmed over clone without registering it. Thanks for clarifying!

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 11, 2017

Other than one bit I don't understand it looks fine and I'll merge once the tests pass.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 11, 2017

Needs a bunch of tests too, so not quite ready yet.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 11, 2017

Let's get this merged now since tests are failing on master holding up other PRs. I'll add tests later.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 11, 2017

Agreed. Merging now.

@jlstevens jlstevens merged commit 8153e45 into master Apr 11, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on master at 79.004%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the dynamic_reindex_fix branch Apr 11, 2017

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