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

Adjust line to account for exclusive upper bounds #330

Merged
merged 1 commit into from
Apr 28, 2017

Conversation

jbcrail
Copy link
Contributor

@jbcrail jbcrail commented Apr 27, 2017

When extending a line, I decremented the upper bounds for both x and y.

Fix #328

When extending a line, I decremented the upper bounds for both x and y.

Fix holoviz#328
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Thanks! Can you explain the reasoning behind this fix? It's clear a change needs to be made, but changing xmax and ymax affects the slope of the line segment, not just its length, and I just want to make sure how you chose this fix rather than leaving the maxes as they are but then changing the comparison. I would guess the correct answer can be determined by hand-plotting some very short lines in small arrays, and if so it would be good to include those here to demonstrate why this is the correct fix. Or is that already what's in the tests (not sure where those matrices came from)?

@jbcrail
Copy link
Contributor Author

jbcrail commented Apr 28, 2017

I tried two strategies for this fix. I focused on extend_line since it was the only method that used the bounds, instead of just passing the value along. My intent with both strategies was to shorten the bounding box for the clipped area.

First, I changed the comparisons in _compute_outcode for both maximum bounds from > to >=. This seemed like the least intrusive approach while still accounting for the exclusive upper bounds. Unfortunately, the while loop in extend_line never exited for several tests.

Second, I decremented the maximum bounds prior to clipping the segment. This approach worked better and is the one in this PR. The modified tests accounted for the newly clipped segments at the exclusive upper bounds. For instance, one test had a top-left to bottom-right line segment that was clipped in the bottom-right corner as expected after the fix.

I missed the change to the slope of the line segment. Ideally, I'd prefer the first strategy but I don't know the code or the algorithms well enough to know what changes are needed to make it work.

@jbednar
Copy link
Member

jbednar commented Apr 28, 2017

Let's go ahead and merge this to avoid the segfaults, but I'd still like to compare to an example worked out on paper before release.

@jbednar jbednar merged commit f158168 into holoviz:master Apr 28, 2017
@jbcrail jbcrail deleted the extend_line_segfault branch April 28, 2017 13:36
jbcrail added a commit to jbcrail/datashader that referenced this pull request Sep 27, 2017
This reverts to exclusive ranges both manual and auto for all glyphs
(points/line) without introducing regressions of holoviz#318, holoviz#330, and holoviz#343.

I refactored several tests to make xarray coordinate indices easier to
read and more explicit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants