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

ENH: dispatch(Field, collections.Mapping) for compute_up #1467

Merged
merged 2 commits into from
Apr 4, 2016

Conversation

llllllllll
Copy link
Member

I also re-exported broadcast_collect through the top-level api, some tests were importing it from there but I cannot understand where it gets there from. These passed on travis but not locally for me so I assume it was coming from and import * on one of the backends

@llllllllll llllllllll force-pushed the field-on-mappings branch 3 times, most recently from 00d772a to 6d7b4e5 Compare April 1, 2016 07:31
@kwmsmith
Copy link
Member

kwmsmith commented Apr 1, 2016

With a little grepping, there are a few other places where we're @dispatching on dict rather than Mapping. Thoughts on converting these as well?

$ ag --python '^@dispatch.*dict'
blaze/compute/core.py
381:@dispatch(Expr, dict)
454:@dispatch(Field, dict)

blaze/compute/python.py
771:@dispatch(Field, dict)

blaze/expr/core.py
368:@dispatch((tuple, list), dict)
373:@dispatch(Node, dict)
386:@dispatch(object, dict)

blaze/expr/expressions.py
342:@dispatch(Symbol, dict)

blaze/interactive.py
185:@dispatch(_Data, dict)

@llllllllll
Copy link
Member Author

I will go over these cases and make sure the conversion makes sense. Also, fun think that I learned while doing this, collections.abc.Mapping does not implement __subclasshook__, so classes must be explicitly registered or actual subclasses. I think it is because the interface is ambigious with a sequence, for example, list implements iter, len, and getitem.

@llllllllll
Copy link
Member Author

I also found some isinstance checks that I think are improved by checking against Mapping

@llllllllll llllllllll force-pushed the field-on-mappings branch 4 times, most recently from 61cc5d6 to 8bbef0b Compare April 1, 2016 19:52
@llllllllll
Copy link
Member Author

ping @kwmsmith tests are passing

@kwmsmith
Copy link
Member

kwmsmith commented Apr 4, 2016

LGTM, thanks.

@kwmsmith kwmsmith merged commit f8ee95e into blaze:master Apr 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants