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

Implemented DynamicMap.map method #1240

Merged
merged 1 commit into from Apr 10, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Apr 3, 2017

Implements the final DynamicMap method which was not yet lazy as outlined in #422.

@philippjfr philippjfr added the API label Apr 3, 2017

@philippjfr philippjfr added this to the v1.7.0 milestone Apr 3, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 3, 2017

Ready to review. get_dynamic_item is still not ideal, but I much prefer not using .map for this, since I think .map is very useful functionality for DynamicMap, since it flexibly let's you apply operations and function to specific objects returned by the DynamicMap.

@philippjfr philippjfr referenced this pull request Apr 4, 2017

Closed

Counter mode deprecation #969

0 of 5 tasks complete
el = map_obj.select(['DynamicMap', 'HoloMap'], **dims)
Traverses the supplied object selecting the requested key on
all HoloMap and DynamicMap objects.
"""

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2017

Member

It would be nice to have a doctest style example here (even if it isn't a working one). Just be sure that it doesn't get picked up as one and then fail!

I understand the problem with a working doctest comes down to circular imports.

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2017

Member

Is map_obj mutated, cloned or is the result potentially a little bit of both?

else:
el = None
el = map_obj
return key, el

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2017

Member

The reason why key is also returned should be explained in the docstring. Why isn't this the input key?

if d in map_obj.kdims}
key = tuple(dims.get(d.name) for d in map_obj.kdims)
el = map_obj.select(map_specs, **dims)
elif map_obj._deep_indexable and not any(map_obj.matches(spec) for spec in overlay_specs):

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2017

Member

I don't understand this condition whereas the other ones do make sense to me. Maybe a comment to explain what this branch is for?

This comment has been minimized.

@philippjfr

philippjfr Apr 4, 2017

Member

Hmm, there might be a better way to express it, it's recurses into all container objects excluding (Nd)Overlay types.

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2017

Member

Ah, I see. I guess this is because you can apply select at the overlay level and you don't need to apply it to the elements here.

else:
deep_mapped = self.clone() if clone else self
for k, v in deep_mapped.data.items():
deep_mapped[k] = v.map(map_fn, specs, clone)

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2017

Member

How do we know it is safe to use deep_mapped.data.items()?

If it is self or self.clone() it should be fine but what if map_fn returns something where this isn't valid? deep_mapped[k] = v.map(map_fn, specs, clone) also suggests that it is expected to be an NdMapping of some sort..

This comment has been minimized.

@philippjfr

philippjfr Apr 4, 2017

Member

It seems in the non-dynamic version we first map the items and then apply it to itself if it applies, so I'll do that here.

deep_mapped[k] = v.map(map_fn, specs, clone)
from ..util import Dynamic
def dynamic_map(obj):

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2017

Member

I would just call this map_operation as dynamic_map could be confusing!

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 10, 2017

Managed to simplify this a lot.

Recursively replaces elements using a map function when the
specification applies. Extends regular map with functionality
to dynamically apply functions.
"""

This comment has been minimized.

@jlstevens

jlstevens Apr 10, 2017

Member

Wow! That really was a massive simplification. I like it!

I just hope it works as expected and doesn't become complicated again. Perhaps a few more unit tests would help?

This comment has been minimized.

@philippjfr

philippjfr Apr 10, 2017

Member

Added one more test to check the clone=False condition, I generally prefer not adding multiple unit tests that just test end up testing the same code paths and Dimensioned.map is already tested.

This comment has been minimized.

@jlstevens

jlstevens Apr 10, 2017

Member

Ok. As long as you feel map is tested on sufficiently complex examples somewhere, I don't mind.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 10, 2017

Ready to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 10, 2017

Great! Tests are passing so I'll merge as requested.

@jlstevens jlstevens merged commit 7aa3266 into master Apr 10, 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 Coverage increased (+0.01%) to 78.907%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the dynamic_map branch Apr 11, 2017

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