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

Full support of the style system using DynamicMaps #368

Merged
merged 3 commits into from Dec 19, 2015

Conversation

Projects
None yet
2 participants
@jlstevens
Copy link
Contributor

jlstevens commented Dec 19, 2015

This PR fixes a major issue with DynamicMaps, namely that they did not properly support styling. This is because when a DynamicMap is created it may contain no elements meaning that there is nothing to propagate an id to when %%opts or __call__ is used. In other words, every time a value is returned by the callback, it was too late as styling had already been applied.

The approach used here seems to work well but there are things I am not entirely happy with:

  • Using __class__.__name__ to check for DynamicMap. I am fairly sure I'll encounter cyclic import issues if I try to import DynamicMap into options.py.
  • It is a little inefficient as I'm converting from an OptionTree to dictionary format and back to an OptionTree per frame.
  • This is going to generate a ludicrous number of custom ids and option trees if you use the DynamicMap much (one per frame!). Many of these will be identical as well (until the style is changed, if ever).

Hopefully, I can address these issue but I think this PR is very important for the 1.4.1 release. Styling DynamicMap as easily as HoloMap is crucial!

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Dec 19, 2015

Just a thought regarding the excess custom ids.... maybe the custom id/tree could be removed when popped from the cache? Then again, someone could still have a handle on an element that is no longer on the cache.

Python does offer the ability to know how many references there are to an object:

import sys
sys.getrefcount(object) #-- Returns the reference count of the object.

Maybe we could use this to implement a system to garbage collect the custom trees...

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Dec 19, 2015

I'm just brainstorming here but there is another approach to clean-up the custom trees. This is probably very over-engineered but I'm just trying to come up with ideas!

  1. Create an object that looks like a dictionary but is in fact a compressed dictionary: for each unique item it holds, it keeps a list of associated ids.
  2. Use tree comparisons to make sure the trees held are all unique. If not, 'compress' this dictionary object to eliminate duplicate option trees. You could periodically compress, e.g if the dictionary seems to be approaching a ludicrous size, e.g 100,000 items...

This would solve the duplicate option trees but there is no reason the list of integer ids wouldn't be able to grow without limit. This could only be solved by sweeping the namespace for all HoloViews objects, and checking with ids actually are being used (then cleaning up the trees associated with the ones no longer in use).

Technically possible (using globals() or the IPython namespace) but really a horrible hack. Could be made into a utility I suppose that exists to be used only when needed...

Edit 1: This suggestion is really equivalent to storing the 'inverted' dictionary where the trees are keys (assuming they are hashable, which I think they are) with lists of ids as values which you keep updated. Then it is all about doing the lookup from id to tree...

Edit 2: Not quite. Even if a tree is hashable, the hash doesn't really map to its 'value'. You would want each tree to have a unique hash (so you can use them as dictionary keys without restriction) but then you would need to enforce 'uniqueness' of the keys explicitly (making sure the values of all the trees are unique).

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Dec 19, 2015

Looks good, glad this could be done fairly easily. The suggestions about squashing the OptionsTrees probably deserve their own issue. I'm happy to merge this now.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Dec 19, 2015

The suggestions about squashing the OptionsTrees probably deserve their own issue.

Sure. I've opened an issue about this above. I think you can go ahead and merge!

philippjfr pushed a commit that referenced this pull request Dec 19, 2015

Philipp Rudiger Philipp Rudiger
Merge pull request #368 from ioam/dynamic_styles
Full support of the style system using DynamicMaps

@philippjfr philippjfr merged commit d3da6fc into master Dec 19, 2015

3 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 decreased (-0.04%) to 70.764%
Details

@jlstevens jlstevens referenced this pull request Dec 19, 2015

Closed

DynamicMap and styles #361

@philippjfr philippjfr deleted the dynamic_styles branch Dec 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.