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

Various Optimizations #359

Merged
merged 6 commits into from
Dec 16, 2015
Merged

Various Optimizations #359

merged 6 commits into from
Dec 16, 2015

Conversation

philippjfr
Copy link
Member

This is a PR for various optimizations on some of the core components of HoloViews.

@philippjfr philippjfr force-pushed the optimization branch 3 times, most recently from 3f02d8f to fda86cb Compare December 15, 2015 03:43
if len(labels) == 1:
return list(labels)[0]
if len(self):
return next(itervalues(self.data)).label
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you still need to check _auxiliary_component as that marks Annotations where you don't want to propagate the label upwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bottom layer should usually not be an annotation but I can just throw a while loop in there that'll iterate until it finds a non-auxiliar component.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that label should be the empty string if the first value (i.e the one we are inspecting) is an _auxiliary_component. Before we were iterating over the NdMapping elements: when you say the bottom layer shouldn't be an annotation, I believe you are thinking about Overlay elements. In other words, I think we need this check for HoloMaps of annotations.

@jlstevens
Copy link
Contributor

One quick comment: the code looks good but seems to repeat itself (almost exactly, but not quite) between group and label. I would consider a utility or this, either a function with an option to get the group or label, or maybe a class with two classmethods get_group and get_label.

Otherwise it looks good and I am happy to merge.

@jlstevens
Copy link
Contributor

Looks good once the tests are passing.

One thing you didn't mention at the start of the PR was how slow the old label/group processing was. I know this will offer some sort of performance improvement but I don't have a feeling for what kind of speed up I can expect and in which situations.

@philippjfr
Copy link
Member Author

This PR is ready for merge now that the tests are passing.

jlstevens added a commit that referenced this pull request Dec 16, 2015
@jlstevens jlstevens merged commit 3084b68 into master Dec 16, 2015
@philippjfr
Copy link
Member Author

One thing you didn't mention at the start of the PR was how slow the old label/group processing was. I know this will offer some sort of performance improvement but I don't have a feeling for what kind of speed up I can expect and in which situations.

For very nested datastructures it saves a lot of time. I tested it on plotting a HoloMap of 1000 Overlays each containing 100 Curves, in that example it saved 120/130 seconds.

@jlstevens
Copy link
Contributor

Thanks. That is a plausible holomap and 120 seconds is a significant saving - although you didn't say how long it took in total. :-)

Edit: Philipp tells me it saved 120 seconds from 130 seconds. A huge improvement indeed!

@philippjfr philippjfr deleted the optimization branch December 20, 2015 16:58
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants