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

Gridmatrix diagonal types #1194

Merged
merged 3 commits into from Mar 14, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Mar 11, 2017

Allowed different types along the diagonal of a gridmatrix. Also lifted the restriction on duplicate dimensions on a Dimensioned object. While I can sort of see why we did that, plotting a variable against itself can be a valid thing to do, e.g. to see the distribution of a variable as shown along the diagonal here (which is now also a holoviews-contrib gallery example):

http://bokeh.pydata.org/en/latest/docs/gallery/iris_splom.html

@@ -758,8 +758,12 @@ def _process(self, p, element, ranges={}):
for d1, d2 in permuted_dims:
if d1 == d2:
if p.diagonal_type is not None:
values = element.dimension_values(d1)
el = p.diagonal_type(values, vdims=[d1])
if p.diagonal_type._1d:

This comment has been minimized.

@jlstevens

jlstevens Mar 12, 2017

Member

What is this _1d attribute on elements?

This comment has been minimized.

@philippjfr

philippjfr Mar 12, 2017

Member

It makes the distinction between Elements that will automatically fill in the x-coordinates and ones that simply render a distribution, e.g. Scatter/Curve vs Distribution/BoxWhisker/Spikes.

This comment has been minimized.

@jlstevens

jlstevens Mar 12, 2017

Member

I'm not sure I like this approach. When was it added and how much is it used?

As far as I can tell this is a class attribute and I'm coming to the conclusion there are lots of properties (such as this one) we want to query at the level of the element type. I would rather not have an undocumented API consisting of all sorts of random class attributes all specified on the element classes.

Instead I'm thinking there could be a set of utilities (either in util or even a new file) that is all about specifying the properties of Elements in a public way. These could be a bunch of predicate functions (with docstrings to explain what the properties mean). E.g the above might be something like:

if util.is_distribution1D(p.diagonal_type):
...

If nothing else, the issue here is that I had no idea what _1d indicates from the name!

This comment has been minimized.

@philippjfr

philippjfr Mar 12, 2017

Member

That does not seem extensible. If I want to declare a new Element type, which has one of these properties how would I tell is_distribution1D about that? As far as I'm aware this is the only class attribute like this and is defined like this on Dataset:

    # In the 1D case the interfaces should not automatically add x-values
    # to supplied data
    _1d = False

This comment has been minimized.

@jlstevens

jlstevens Mar 12, 2017

Member

That does not seem extensible. If I want to declare a new Element type, which has one of these properties how would I tell is_distribution1D about that?

Well, nothing says you'll remember to override _1d when you define your new class! It is good to hear it is one of the only things like this - maybe it just needs a better name then?

This comment has been minimized.

@philippjfr

philippjfr Mar 13, 2017

Member

element._properties sounds like a reasonable suggestion. For the time being _integer_indexable_1d isn't really correct though, it only adds an integer index if you don't supply x-values yourself, _add_1d_index maybe?

This comment has been minimized.

@jlstevens

jlstevens Mar 13, 2017

Member

_augmentable_1d_index? or _allows_implicit_1d_index? or _auto_1d_indexable?

Tricky!

This comment has been minimized.

@philippjfr

philippjfr Mar 13, 2017

Member

_augmentable_1d_index

Don't really know what that means.

_allows_implicit_1d_index

Allows doesn't seem right, it's adding an index if you don't provide it.

_auto_1d_indexable

This comes closest out of your suggestions.

This comment has been minimized.

@jlstevens

jlstevens Mar 13, 2017

Member

I thought that might be the best one. How about _auto_indexable_1d?

This comment has been minimized.

@philippjfr

philippjfr Mar 13, 2017

Member

Sounds good, I'll go with that.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 13, 2017

I'm going to open a separate issue about the add index flag because currently it's being (ab)used in two ways, 1) to find out whether to add a 1D integer index if no x-values have been supplied and 2) whether an Element can represent a distribution of values without an index (i.e. no kdims). Ideally I'd just use the default number of kdims to determine 2) but it seems Spikes currently does not correctly follow that pattern.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 13, 2017

Please do file an issue. The _properties suggestion would let you decouple these concepts as necessary.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 14, 2017

Now the tests are passing, I'll merge.

@jlstevens jlstevens merged commit e9bd6e6 into master Mar 14, 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 decreased (-0.002%) to 78.554%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the gridmatrix_diagonal_types branch Apr 19, 2017

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