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

Projects
None yet
2 participants
@llllllllll
Member

llllllllll commented Apr 1, 2016

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 quantopian:field-on-mappings branch 3 times, most recently from 00d772a to 6d7b4e5 Apr 1, 2016

@kwmsmith

This comment has been minimized.

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

This comment has been minimized.

Member

llllllllll commented Apr 1, 2016

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 llllllllll force-pushed the quantopian:field-on-mappings branch from d709010 to 94a2950 Apr 1, 2016

@llllllllll

This comment has been minimized.

Member

llllllllll commented Apr 1, 2016

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

@llllllllll llllllllll force-pushed the quantopian:field-on-mappings branch 4 times, most recently from 61cc5d6 to 8bbef0b Apr 1, 2016

@llllllllll

This comment has been minimized.

Member

llllllllll commented Apr 1, 2016

ping @kwmsmith tests are passing

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Apr 4, 2016

LGTM, thanks.

@kwmsmith kwmsmith merged commit f8ee95e into blaze:master Apr 4, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.9%) to 88.302%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment