Vbar hbar #4815

Merged
merged 36 commits into from Jul 20, 2016

Conversation

Projects
None yet
5 participants
@ericmjl
Contributor

ericmjl commented Jul 16, 2016

No description provided.

@canavandl

This comment has been minimized.

Show comment
Hide comment
@canavandl

canavandl Jul 16, 2016

Contributor

I think may have accidently checked-in Quad.html and Vbar.html? They'll probably have to be removed.

Contributor

canavandl commented Jul 16, 2016

I think may have accidently checked-in Quad.html and Vbar.html? They'll probably have to be removed.

@ericmjl

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 16, 2016

Contributor

Thanks for letting me know. I will take care of that.

Contributor

ericmjl commented Jul 16, 2016

Thanks for letting me know. I will take care of that.

@ericmjl

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 17, 2016

Contributor

@canavandl just getting back to the thing you mentioned - where did you put Quad.html and Vbar.html?

Contributor

ericmjl commented Jul 17, 2016

@canavandl just getting back to the thing you mentioned - where did you put Quad.html and Vbar.html?

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 19, 2016

Member

@ericmjl I think @canavandl is saying that you accidentally checked those files in to the branch (you can see them in "Files Changed" tab). They need to be removed

Member

bryevdv commented Jul 19, 2016

@ericmjl I think @canavandl is saying that you accidentally checked those files in to the branch (you can see them in "Files Changed" tab). They need to be removed

bokeh/models/glyphs.py
+ """)
+
+ fill_props = Include(FillProps, use_prefix=False, help="""
+ The %s values for the rectangles.

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

suggest updating both of these to "horizontal bars" instead of "rectangles"

@bryevdv

bryevdv Jul 19, 2016

Member

suggest updating both of these to "horizontal bars" instead of "rectangles"

bokeh/models/glyphs.py
+ _args = ('y', 'height', 'left', 'right')
+
+ y = NumberSpec(help="""
+ The y-coordinates of the centers of the bars.

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

suggest adding "horizontal" before "bars"

@bryevdv

bryevdv Jul 19, 2016

Member

suggest adding "horizontal" before "bars"

bokeh/models/glyphs.py
+
+ x = NumberSpec(help="""
+ The x-coordinates of the centers of the bars.
+ """)

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

suggest adding "vertical" before "bars"

@bryevdv

bryevdv Jul 19, 2016

Member

suggest adding "vertical" before "bars"

bokeh/models/glyphs.py
+
+ bottom = NumberSpec(help="""
+ The y-coordinates of the bottom edges.
+ """)

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

we could be opinionated here, it's possible to add default=0 and that would make the bottom=0 unnecessary for bars that do happen to start at 0

@bryevdv

bryevdv Jul 19, 2016

Member

we could be opinionated here, it's possible to add default=0 and that would make the bottom=0 unnecessary for bars that do happen to start at 0

This comment has been minimized.

@ericmjl

ericmjl Jul 19, 2016

Contributor

I'm not quite sure about this. What's the intended use case for VBar and HBar? Is likely isn't a user passing in data, in which case I think it makes more sense for the user to be explicit about what the top and bottom values should be, would that be right?

@ericmjl

ericmjl Jul 19, 2016

Contributor

I'm not quite sure about this. What's the intended use case for VBar and HBar? Is likely isn't a user passing in data, in which case I think it makes more sense for the user to be explicit about what the top and bottom values should be, would that be right?

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

I'm not suggesting adding a default for top. I expect VBar will be used to easily create bar-like charts using bokeh.plotting. The suggestion is merely to make what (I assume is) the common case -- bars starting from y=0 slightly easier to spell.

plot.vbar(x=[1, 2, 3], width=0.5, top=[1,2,3])

instead of

plot.vbar(x=[1, 2, 3], width=0.5, bottom=0, top=[1,2,3])

It's s minor thing, but common cases are the most important places for minor improvements. Of course if the bars don't start at zero the user can still supply bottom explicitly, that's no different.

@bryevdv

bryevdv Jul 19, 2016

Member

I'm not suggesting adding a default for top. I expect VBar will be used to easily create bar-like charts using bokeh.plotting. The suggestion is merely to make what (I assume is) the common case -- bars starting from y=0 slightly easier to spell.

plot.vbar(x=[1, 2, 3], width=0.5, top=[1,2,3])

instead of

plot.vbar(x=[1, 2, 3], width=0.5, bottom=0, top=[1,2,3])

It's s minor thing, but common cases are the most important places for minor improvements. Of course if the bars don't start at zero the user can still supply bottom explicitly, that's no different.

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

And of course being explicit with bottom=0would also still be fine, but it makes things a little shorter for those that would like them to be

@bryevdv

bryevdv Jul 19, 2016

Member

And of course being explicit with bottom=0would also still be fine, but it makes things a little shorter for those that would like them to be

This comment has been minimized.

@ericmjl

ericmjl Jul 19, 2016

Contributor

Ah, I see. My apologies for the previous comment - I had a grammatical breakdown.

I was thinking about a particular case that isn't assumed with the default bottom=0:

plot.vbar(x=[1, 2, 3], width=0.5, bottom=[-3, 0, 0], top=[0, 2, 3])

In this case, there is one negative bar. A user would have to explicitly specify that the bottom is negative, then.

Ok, on thinking about it, I think having a default bottom=0 makes sense. I'll add this in, and likewise for HBar (left=0).

@ericmjl

ericmjl Jul 19, 2016

Contributor

Ah, I see. My apologies for the previous comment - I had a grammatical breakdown.

I was thinking about a particular case that isn't assumed with the default bottom=0:

plot.vbar(x=[1, 2, 3], width=0.5, bottom=[-3, 0, 0], top=[0, 2, 3])

In this case, there is one negative bar. A user would have to explicitly specify that the bottom is negative, then.

Ok, on thinking about it, I think having a default bottom=0 makes sense. I'll add this in, and likewise for HBar (left=0).

tests/glyphs/HBar.py
+ y=y,
+ right=x,
+ )
+)

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

small style suggestion: this could all easily fit on one line:

source = ColumnDataSource(dict(y=y, right=x))
@bryevdv

bryevdv Jul 19, 2016

Member

small style suggestion: this could all easily fit on one line:

source = ColumnDataSource(dict(y=y, right=x))
tests/glyphs/VBar.py
+ top=y,
+ )
+)
+

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

same comment, suggest one line for source definition

@bryevdv

bryevdv Jul 19, 2016

Member

same comment, suggest one line for source definition

+ # w2 = @width[i]/2
+ # pts.push([@x[i]-w2, @bottom[i], @x[i]+w2, @top[i], {'i': i}])
+ # index.load(pts)
+ # return index

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

did you want to continue this in this PR? If not, let's just delete this and the other block in the other file. I'm almost always -1 on checking in commented-out code

@bryevdv

bryevdv Jul 19, 2016

Member

did you want to continue this in this PR? If not, let's just delete this and the other block in the other file. I'm almost always -1 on checking in commented-out code

This comment has been minimized.

@ericmjl

ericmjl Jul 19, 2016

Contributor

_index_data is needed for hit testing, right? I'm going to need a lesson from you on what hit testing is all about, btw 😛. In any case, I will remove it and the other one first.

@ericmjl

ericmjl Jul 19, 2016

Contributor

_index_data is needed for hit testing, right? I'm going to need a lesson from you on what hit testing is all about, btw 😛. In any case, I will remove it and the other one first.

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

Yup, all this is to support hit testing, which we can certainly add in a follow-on later. It's potentially possible to just add "brute force" hit testing without an index, too. We can discuss details later

@bryevdv

bryevdv Jul 19, 2016

Member

Yup, all this is to support hit testing, which we can certainly add in a follow-on later. It's potentially possible to just add "brute force" hit testing without an index, too. We can discuss details later

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 19, 2016

Member

@ericmjl I left a few other small comments otherwise this is looking great. This will be a nice add!

Member

bryevdv commented Jul 19, 2016

@ericmjl I left a few other small comments otherwise this is looking great. This will be a nice add!

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 19, 2016

Member

@ericmjl one other suggestion: our CI tests kick off on ever push, and we only have 5 workers. Best to collect all the commits locally and then push at the end

Member

bryevdv commented Jul 19, 2016

@ericmjl one other suggestion: our CI tests kick off on ever push, and we only have 5 workers. Best to collect all the commits locally and then push at the end

@ericmjl

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 19, 2016

Contributor

@bryevdv yes, i'm sorry about the multiple pushes. thanks for letting me know!

Contributor

ericmjl commented Jul 19, 2016

@bryevdv yes, i'm sorry about the multiple pushes. thanks for letting me know!

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 19, 2016

Member

no apologies needed, help and guidance is our responsibility

Member

bryevdv commented Jul 19, 2016

no apologies needed, help and guidance is our responsibility

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 19, 2016

Member

FYI while I'm +1 on defaulting bottom=0 I'm +0 on defaulting left=0. I'll leave that to you to decide

Member

bryevdv commented Jul 19, 2016

FYI while I'm +1 on defaulting bottom=0 I'm +0 on defaulting left=0. I'll leave that to you to decide

bokeh/models/glyphs.py
+
+ bottom = NumberSpec(help="""
+ The y-coordinates of the bottom edges.
+ """, default=0)

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

OK, being super picky here :) but every other property puts the help last, I think it would be good to be consistent, so devs are never surprised:

 +    bottom = NumberSpec(default=0, help="""
 +    The y-coordinates of the bottom edges.
 +    """)

@bryevdv

bryevdv Jul 19, 2016

Member

OK, being super picky here :) but every other property puts the help last, I think it would be good to be consistent, so devs are never surprised:

 +    bottom = NumberSpec(default=0, help="""
 +    The y-coordinates of the bottom edges.
 +    """)

This comment has been minimized.

@ericmjl

ericmjl Jul 19, 2016

Contributor

I'm sorry! I should have looked more carefully at the conventions.

@ericmjl

ericmjl Jul 19, 2016

Contributor

I'm sorry! I should have looked more carefully at the conventions.

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

Not at all, it's not the sort of thing that's easy to notice as a patter, again that's why we are here to help!

@bryevdv

bryevdv Jul 19, 2016

Member

Not at all, it's not the sort of thing that's easy to notice as a patter, again that's why we are here to help!

@ericmjl

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 19, 2016

Contributor

@bryevdv I think it makes sense to include left=0 as a default, for consistency purposes. Most use cases - data are positively valued, and visually bottom and left are used for zeros.

Contributor

ericmjl commented Jul 19, 2016

@bryevdv I think it makes sense to include left=0 as a default, for consistency purposes. Most use cases - data are positively valued, and visually bottom and left are used for zeros.

bokeh/models/glyphs.py
+
+ left = NumberSpec(help="""
+ The x-coordinates of the left edges.
+ """, default=0)

This comment has been minimized.

@bryevdv

bryevdv Jul 19, 2016

Member

same comment about default being first

@bryevdv

bryevdv Jul 19, 2016

Member

same comment about default being first

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 20, 2016

Member

Ok, I forgot to mention the defaults on the JS side (in the @define blocks) also need to be updated to match, and also the python unit tests need to be updated as well:

https://travis-ci.org/bokeh/bokeh/jobs/145977715#L2468-L2491

Member

bryevdv commented Jul 20, 2016

Ok, I forgot to mention the defaults on the JS side (in the @define blocks) also need to be updated to match, and also the python unit tests need to be updated as well:

https://travis-ci.org/bokeh/bokeh/jobs/145977715#L2468-L2491

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 20, 2016

Member

@ericmjl there is one last test still failing, I'm not sure what it is offhand, but I will look into it this afternoon

Member

bryevdv commented Jul 20, 2016

@ericmjl there is one last test still failing, I'm not sure what it is offhand, but I will look into it this afternoon

@ericmjl

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 20, 2016

Contributor

@bryevdv I've been trying to debug it too this morning, but I think I'm running into the limits of my knowledge of the bokeh library at this point. Thanks for guiding me along this too, btw!

Contributor

ericmjl commented Jul 20, 2016

@bryevdv I've been trying to debug it too this morning, but I think I'm running into the limits of my knowledge of the bokeh library at this point. Thanks for guiding me along this too, btw!

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 20, 2016

Member

@ericmjl the coffee default values need to be updated to match. The following patch gets the tests passing:

 git diff
diff --git a/bokehjs/src/coffee/models/glyphs/hbar.coffee b/bokehjs/src/coffee/models/glyphs/hbar.coffee
index c176c56..850fdf3 100644
--- a/bokehjs/src/coffee/models/glyphs/hbar.coffee
+++ b/bokehjs/src/coffee/models/glyphs/hbar.coffee
@@ -46,10 +46,10 @@ class HBar extends Glyph.Model

   @mixins ['line', 'fill']
   @define {
-      y:      [ p.NumberSpec   ]
-      height: [ p.DistanceSpec ]
-      left:   [ p.NumberSpec   ]
-      right:  [ p.NumberSpec   ]
+      y:      [ p.NumberSpec     ]
+      height: [ p.DistanceSpec   ]
+      left:   [ p.NumberSpec,  0 ]
+      right:  [ p.NumberSpec     ]
     }

 module.exports =
diff --git a/bokehjs/src/coffee/models/glyphs/vbar.coffee b/bokehjs/src/coffee/models/glyphs/vbar.coffee
index 5ef9ed3..e8e076b 100644
--- a/bokehjs/src/coffee/models/glyphs/vbar.coffee
+++ b/bokehjs/src/coffee/models/glyphs/vbar.coffee
@@ -45,10 +45,10 @@ class VBar extends Glyph.Model

   @mixins ['line', 'fill']
   @define {
-      x:      [ p.NumberSpec   ]
-      width:  [ p.DistanceSpec ]
-      top:    [ p.NumberSpec   ]
-      bottom: [ p.NumberSpec   ]
+      x:      [ p.NumberSpec     ]
+      width:  [ p.DistanceSpec   ]
+      top:    [ p.NumberSpec     ]
+      bottom: [ p.NumberSpec,  0 ]
     }

 module.exports =

Then we should be good to merge.

Member

bryevdv commented Jul 20, 2016

@ericmjl the coffee default values need to be updated to match. The following patch gets the tests passing:

 git diff
diff --git a/bokehjs/src/coffee/models/glyphs/hbar.coffee b/bokehjs/src/coffee/models/glyphs/hbar.coffee
index c176c56..850fdf3 100644
--- a/bokehjs/src/coffee/models/glyphs/hbar.coffee
+++ b/bokehjs/src/coffee/models/glyphs/hbar.coffee
@@ -46,10 +46,10 @@ class HBar extends Glyph.Model

   @mixins ['line', 'fill']
   @define {
-      y:      [ p.NumberSpec   ]
-      height: [ p.DistanceSpec ]
-      left:   [ p.NumberSpec   ]
-      right:  [ p.NumberSpec   ]
+      y:      [ p.NumberSpec     ]
+      height: [ p.DistanceSpec   ]
+      left:   [ p.NumberSpec,  0 ]
+      right:  [ p.NumberSpec     ]
     }

 module.exports =
diff --git a/bokehjs/src/coffee/models/glyphs/vbar.coffee b/bokehjs/src/coffee/models/glyphs/vbar.coffee
index 5ef9ed3..e8e076b 100644
--- a/bokehjs/src/coffee/models/glyphs/vbar.coffee
+++ b/bokehjs/src/coffee/models/glyphs/vbar.coffee
@@ -45,10 +45,10 @@ class VBar extends Glyph.Model

   @mixins ['line', 'fill']
   @define {
-      x:      [ p.NumberSpec   ]
-      width:  [ p.DistanceSpec ]
-      top:    [ p.NumberSpec   ]
-      bottom: [ p.NumberSpec   ]
+      x:      [ p.NumberSpec     ]
+      width:  [ p.DistanceSpec   ]
+      top:    [ p.NumberSpec     ]
+      bottom: [ p.NumberSpec,  0 ]
     }

 module.exports =

Then we should be good to merge.

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jul 20, 2016

Member

@ericmjl all expected tests passing. Thanks for the great PR! Let me know when you'd like to work on the hit-testing and I can help explain what's going on there.

Member

bryevdv commented Jul 20, 2016

@ericmjl all expected tests passing. Thanks for the great PR! Let me know when you'd like to work on the hit-testing and I can help explain what's going on there.

@bryevdv bryevdv merged commit 8627d3b into bokeh:master Jul 20, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@ericmjl

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 21, 2016

Contributor

@bryevdv thanks for guiding me through! Let me get my life back in order next week, and I'll be back up to try hit testing!

Contributor

ericmjl commented Jul 21, 2016

@bryevdv thanks for guiding me through! Let me get my life back in order next week, and I'll be back up to try hit testing!

@damianavila

This comment has been minimized.

Show comment
Hide comment
@damianavila

damianavila Jul 22, 2016

Contributor

@ericmjl @bryevdv is there any issue opened for this PR? Or is it self-contained?

Contributor

damianavila commented Jul 22, 2016

@ericmjl @bryevdv is there any issue opened for this PR? Or is it self-contained?

@ericmjl

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 22, 2016

Contributor

@damianavila I think this is linked to issue #3423. Hit testing is still missing, as mentioned above, but I plan to work on it in a separate PR.

Contributor

ericmjl commented Jul 22, 2016

@damianavila I think this is linked to issue #3423. Hit testing is still missing, as mentioned above, but I plan to work on it in a separate PR.

@damianavila

This comment has been minimized.

Show comment
Hide comment
@damianavila

damianavila Jul 22, 2016

Contributor

Thanks @ericmjl.

Contributor

damianavila commented Jul 22, 2016

Thanks @ericmjl.

@phobson

This comment has been minimized.

Show comment
Hide comment
@phobson

phobson Aug 3, 2016

it would seem to me that the hit testing would be identical to the that of the Quad glyph. am I over-simplifying things? I'm messing with this here: phobson#1

phobson commented Aug 3, 2016

it would seem to me that the hit testing would be identical to the that of the Quad glyph. am I over-simplifying things? I'm messing with this here: phobson#1

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Aug 3, 2016

Member

@phobson yes that would work but the spatial index also has to be built (i.e. the _index_data method also needs to be implemented so that @index exists for _hit_point to make use of). Alternatively, _hit_point could iterate over all the data points and do "point in rect" hit testing by brute force.

Member

bryevdv commented Aug 3, 2016

@phobson yes that would work but the spatial index also has to be built (i.e. the _index_data method also needs to be implemented so that @index exists for _hit_point to make use of). Alternatively, _hit_point could iterate over all the data points and do "point in rect" hit testing by brute force.

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Aug 3, 2016

Member

@phobson as a reference the _index_data for quads is here:

https://github.com/bokeh/bokeh/blob/master/bokehjs/src/coffee/models/glyphs/quad.coffee#L10-L42

The index needs "top, bottom, left, right" coords. vbar has "top, bottom, x, width" and hbar has "y, height, left, right" so the main task in _index_data implementations for vbar and hbar would be to convert "x, width" into "left, right" and "y, height" into "top, bottom" respectively, then the index data method for each would follow what the one for quad does.

Member

bryevdv commented Aug 3, 2016

@phobson as a reference the _index_data for quads is here:

https://github.com/bokeh/bokeh/blob/master/bokehjs/src/coffee/models/glyphs/quad.coffee#L10-L42

The index needs "top, bottom, left, right" coords. vbar has "top, bottom, x, width" and hbar has "y, height, left, right" so the main task in _index_data implementations for vbar and hbar would be to convert "x, width" into "left, right" and "y, height" into "top, bottom" respectively, then the index data method for each would follow what the one for quad does.

@phobson

This comment has been minimized.

Show comment
Hide comment
@phobson

phobson Aug 3, 2016

@bryevdv thanks for the guidance.

I'll spend sometime on this on my own for a while and come back with more complete thoughts and questions -- perhaps with a proper PR.

phobson commented Aug 3, 2016

@bryevdv thanks for the guidance.

I'll spend sometime on this on my own for a while and come back with more complete thoughts and questions -- perhaps with a proper PR.

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Aug 3, 2016

Member

@phobson that would definitely be extremely valuable and appreciated please let us know any questions we can answer

Member

bryevdv commented Aug 3, 2016

@phobson that would definitely be extremely valuable and appreciated please let us know any questions we can answer

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