-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Conversation
Ready to review. |
holoviews/core/util.py
Outdated
el = map_obj.select(['DynamicMap', 'HoloMap'], **dims) | ||
Traverses the supplied object selecting the requested key on | ||
all HoloMap and DynamicMap objects. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is map_obj
mutated, cloned or is the result potentially a little bit of both?
holoviews/core/util.py
Outdated
else: | ||
el = None | ||
el = map_obj | ||
return key, el |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why key is also returned should be explained in the docstring. Why isn't this the input key?
holoviews/core/util.py
Outdated
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, there might be a better way to express it, it's recurses into all container objects excluding (Nd)Overlay types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
holoviews/core/spaces.py
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
holoviews/core/spaces.py
Outdated
deep_mapped[k] = v.map(map_fn, specs, clone) | ||
|
||
from ..util import Dynamic | ||
def dynamic_map(obj): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just call this map_operation
as dynamic_map
could be confusing!
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. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. As long as you feel map
is tested on sufficiently complex examples somewhere, I don't mind.
Ready to merge. |
Great! Tests are passing so I'll merge as requested. |
Implements the final
DynamicMap
method which was not yet lazy as outlined in #422.