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.

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

@philippjfr
Copy link
Member

@philippjfr 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
Copy link
Member Author

@philippjfr 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.

Loading

@philippjfr philippjfr force-pushed the dynamic_reindex_fix branch from 3fa4bf6 to 8ef0b8a Apr 11, 2017
@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()
Copy link
Contributor

@jlstevens jlstevens Apr 11, 2017

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@philippjfr philippjfr Apr 11, 2017

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor

@jlstevens jlstevens Apr 11, 2017

Choose a reason for hiding this comment

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

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

Loading

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Apr 11, 2017

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

Loading

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Apr 11, 2017

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

Loading

@philippjfr
Copy link
Member Author

@philippjfr 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.

Loading

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Apr 11, 2017

Agreed. Merging now.

Loading

@jlstevens jlstevens merged commit 8153e45 into master Apr 11, 2017
4 checks passed
Loading
@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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants