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.

Sign up for GitHub

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

Jump to bottom

BUG: spatial/qhull: get HalfspaceIntersection.dual_points from the correct array #10503

Merged
merged 1 commit into from
Jul 29, 2019
Merged

BUG: spatial/qhull: get HalfspaceIntersection.dual_points from the correct array #10503

merged 1 commit into from
Jul 29, 2019

Conversation

pv
Copy link
Member

@pv pv commented Jul 23, 2019 •
edited
Loading

Qhull stores half-space intersection points in the points array, which
gives the canonical ordering for the dual hull points. qhalf Hx,y o indeed
prints exactly these points.

The order of points in vertex_list may be permuted, and should not be
used here.

Fixes gh-10501

@pv
Copy link
Member Author

pv commented Jul 23, 2019

cc @haudren

@pv pv mentioned this pull request Jul 23, 2019
Closed
@pv pv added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Jul 23, 2019
@haudren
Copy link
Contributor

haudren commented Jul 23, 2019

Very interesting bug. I must say I never used that feature extensively, I was mostly interested in getting the intersection points for my use cases (in any order). It's been a while since I touched that code, so I don't have any comments to add, thanks for fixing this!

ilya-palachev
ilya-palachev approved these changes Jul 23, 2019
View reviewed changes
Copy link

@ilya-palachev ilya-palachev left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@pv
BUG: spatial/qhull: get HalfspaceIntersection.dual_points from the co…
f2c8874 
…rrect array

Qhull stores half-space intersection points in the `points` array, which
gives the canonical ordering for the dual hull points.

The order of points in `vertex_list` may be permuted, and should not be
used here.
@ilya-palachev
Copy link

Hello! Are there any plans to merge this?

haudren
haudren approved these changes Jul 29, 2019
View reviewed changes
Copy link
Contributor

@haudren haudren left a comment

Choose a reason for hiding this comment

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

I'm back on my computer, looks good to me :)

@pv
Copy link
Member Author

pv commented Jul 29, 2019 •
edited
Loading

@ilya-palachev: yes, but there's time to next release. If you need it now, git fetch origin refs/pull/10503/merge; git checkout FETCH_HEAD (the patch may apply also on top last stable release, didn't check).

@pv pv merged commit 1e3048f into scipy:master Jul 29, 2019
@pv pv mentioned this pull request Jul 29, 2019
Merged
@tylerjereddy tylerjereddy added this to the 1.4.0 milestone Jul 30, 2019
@ilya-palachev
Copy link

@ilya-palachev: yes, but there's time to next release. If you need it now, git fetch origin refs/pull/10503/merge; git checkout FETCH_HEAD (the patch may apply also on top last stable release, didn't check).

OK, thank you @pv !

@rgommers rgommers removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Aug 7, 2019
@tylerjereddy tylerjereddy mentioned this pull request Sep 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haudren haudren haudren approved these changes

@ilya-palachev ilya-palachev ilya-palachev approved these changes

Assignees
No one assigned
Labels
scipy.spatial
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.

BUG: scipy.spatial.HalfspaceIntersection works incorrectly
5 participants
@pv @haudren @ilya-palachev @rgommers @tylerjereddy