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

ENH: Convexhull visiblefacets #9721

Merged
merged 8 commits into from Mar 23, 2019
Merged

ENH: Convexhull visiblefacets #9721

merged 8 commits into from Mar 23, 2019

Conversation

@rtshort
Copy link
Contributor

rtshort commented Jan 27, 2019

This pull request has three unrelated changes to qhull interface.

  1. The important change. Qhull has the ability to create a list of facets in the convex hull that are visible from a point via the QGn and QG-n flags. The current qhull.pyx allows the user to use the options but doesn't provide enough information to actually extract the visible info. The qhull library returns a list of facets and each facet has a "good" flag. All this modification does is return the "good" flag for each facet in an array called good.

I am working up some unit tests, but there are some test programs in https://github.com/rtshort/spatial.git.

  1. In both the Delaunay and Voronoi classes I added a flag to the class that indicates whether this was a furthest site diagram or not. I did this because carrying the input flag furthest_site along into the downstream processing (e.g. plotting or pole of inaccessibility computations) was error prone and clunky. This is not terribly important, just a convenience.

  2. There seems to be an error in the docstrings for Voronoi - the docs said 1 finite and 4 finite ridges. I believe I corrected this.

@tylerjereddy tylerjereddy changed the title Convexhull visiblefacets ENH: Convexhull visiblefacets Jan 28, 2019
Copy link
Contributor

tylerjereddy left a comment

Is this in a state where it is ready for me to test interactively using the PR branch?

The CI failures aren't related to this PR -- you'll want to rebase on the latest master branch the next time you get around to making adjustments here so you get the latest fixes for the testing infrastructure.

scipy/spatial/qhull.pyx Outdated Show resolved Hide resolved
scipy/spatial/qhull.pyx Outdated Show resolved Hide resolved
scipy/spatial/qhull.pyx Show resolved Hide resolved
@rtshort

This comment has been minimized.

Copy link
Contributor Author

rtshort commented Feb 2, 2019

I reverted the changes for the two minor updates. I don't seem to be able to edit the pull request, but for now only the major issue is in the fork. I will do the other two one at a time when we finish this.

Copy link
Contributor

tylerjereddy left a comment

This is looking pretty cool in my initial local testing. See my example code below that produces a plot and may be a suitable draft / start for something we could put in the docstring to help users understand this. I have one question based on messing around with this -- does it make sense that no facets are visible when the query point n is inside the object / ConvexHull? That got me thinking a bit.

import numpy as np
import scipy
from scipy.spatial import ConvexHull, convex_hull_plot_2d
import matplotlib
matplotlib.use('Agg')
import matplotlib.pyplot as plt

fig = plt.figure()
ax = fig.add_subplot(1,1,1)

# generators is a square with
# a point above it
generators = np.array([[0.2, 0.2],
                       [0.2, 0.4],
                       [0.4, 0.4],
                       [0.4, 0.2],
                       [0.3, 0.6]])

# assess visibility relative to the
# external (index 4) point, which is
# excluded from the hull via QG4
# Qhull option
hull = ConvexHull(points=generators,
                  qhull_options='QG4')

print(hull.simplices)
print(hull.good)

# highlight the visible facet(s)
for visible_facet in hull.simplices[hull.good]:
    ax.plot(hull.points[visible_facet, 0],
            hull.points[visible_facet, 1],
            color='violet',
            lw=6)
convex_hull_plot_2d(hull, ax=ax)

fig.savefig('hull_simplex_visibility.png', dpi=300) 
[[1 0]
 [1 2]
 [3 0]
 [3 2]]
[False  True False False]

hull_simplex_visibility

On the topic of starting to write unit tests, I'll just quickly draft a simple pseudo-example I haven't tried yet as a sort of template to be placed in the scipy/spatial/tests/test_qhull.py (probably?) testing infrastructure:

def test_hull_facet_visibility():
   # repeat the example code above
  # and make an assertion about it
  expected = np.array([False, True, False, False], dtype=bool)
  actual = hull.good
  assert_equal(actual, expected)
scipy/spatial/qhull.pyx Outdated Show resolved Hide resolved
@tylerjereddy tylerjereddy added this to the 1.3.0 milestone Feb 11, 2019
@rtshort

This comment has been minimized.

Copy link
Contributor Author

rtshort commented Feb 12, 2019

I made a passel of changes, hopefully everything we discussed. A list follows. I am not especially git/github literate so I hope I didn't dork things up horribly.

typed memoryviews: You didn't ask for a change, but doesn't make sense to have two different approaches and I am no way no how competent to make these changes.

docstrings: Moved "good" stuff below the coplanar stuff.
Added "good" example to ConvexHull - just stole yours and made it a little shorter.
Enhanced the "good" documentation and changed "indicated" to "indicating" (good catch).
I have a little trepidation here since I am providing my interpretation of the very sparse
qhull documentation, but overall I think this is an improvement.

Added "versionadded". Since I don't know what version (if ever) this stuff will be added this is incomplete.

I took the good attribute out of Delaunay. All facets are always not-good all the time.

I added unit tests.

Not done: I have not played with incremental stuff. It should work but obviously should be tested. I will do that over the next few days.

@rtshort

This comment has been minimized.

Copy link
Contributor Author

rtshort commented Feb 13, 2019

Note: Incremental processing only works if "restart=True". This will take a little thought.

scipy/spatial/qhull.pyx Outdated Show resolved Hide resolved
scipy/spatial/qhull.pyx Outdated Show resolved Hide resolved
scipy/spatial/tests/test_qhull.py Outdated Show resolved Hide resolved
@tylerjereddy

This comment has been minimized.

Copy link
Contributor

tylerjereddy commented Feb 13, 2019

Note: Incremental processing only works if "restart=True". This will take a little thought.

Are you saying that the visibility feature you're adding here only works with incremental when restart=True? Does it raise an error or segfault or silently return something incorrect otherwise?

@rtshort

This comment has been minimized.

Copy link
Contributor Author

rtshort commented Feb 14, 2019

The facets are updated but apparently the "good" array needs some work to update it as well. I probably won't try to fix it. I have no need for this feature and it is going to take a lot of digging to figure it out.

For the next six or eight weeks the time I will have available to work on this is going to be very slim. I expect I will just have to drop this whole effort until the other project is finished.

@tylerjereddy

This comment has been minimized.

Copy link
Contributor

tylerjereddy commented Feb 15, 2019

We could think about adding the feature only for the non-incremental case initially & open an issue with the idea to support the incremental addition of points in combination with visibility later on. Divide and conquer, maybe!

If you are too swamped I could also try to chip away at the PR a bit by pushing a few commits here and there & preserving your git history for now. I don't think there's a big rush on the feature in any case.

@rtshort

This comment has been minimized.

Copy link
Contributor Author

rtshort commented Feb 19, 2019

Sorry for the slow response. Travel. I will be doing a lot of that for the next couple of months.

I think your notion of how to do the incremental thing is a good one. Document it and leave it for later.

I agree that this feature is not a rush. It is very possible that I am the only person who ever wanted it! I will have little free time until May or so but that isn't the same as none. You are more than welcome to poke and prod at the repo.

@tylerjereddy

This comment has been minimized.

Copy link
Contributor

tylerjereddy commented Mar 3, 2019

I rebased & pushed a commit that should allow the new ConvexHull visibility unit tests to pass & that restores the PR to a state where only the changes focused on the major new feature (ConvexHull facet visibility) should be present in the diff.

Still on the to-do:

  • clean up the docstring additions a bit
  • decide how to proceed re: incremental visibility
@tylerjereddy

This comment has been minimized.

Copy link
Contributor

tylerjereddy commented Mar 7, 2019

Ok, I've added support for incremental visibility updates & substantially expanded the unit tests for this and other cases.

I tried to clean up the docstring a bit too.

This is ready for broader review I think.

@tylerjereddy

This comment has been minimized.

Copy link
Contributor

tylerjereddy commented Mar 7, 2019

Looks like there's one issue specific to 32-bit linux.

scipy/spatial/qhull.pyx Outdated Show resolved Hide resolved
scipy/spatial/tests/test_qhull.py Outdated Show resolved Hide resolved
@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Mar 8, 2019

This is ready for broader review I think.

Overall this looks like a good addition. html docs also look fine. Were you looking for specific input? If both of you are happy and there are no comments for some days, I'd say go ahead and merge (after test failure is fixed)

@rtshort

This comment has been minimized.

Copy link
Contributor Author

rtshort commented Mar 9, 2019

Sorry I have been off the grid for a bit. I have some time this weekend and will look at what you have done but it looks great to me at a glance. One question though: how do I get the html from the code?

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Mar 9, 2019

how do I get the html from the code?

if you want to build the html docs for yourself: cd doc && make html.

@tylerjereddy

This comment has been minimized.

Copy link
Contributor

tylerjereddy commented Mar 9, 2019

I'll aim to merge in about two weeks (March 23) if there are no unresolved issues brought up before then.

I tried a little property-based testing to probe for major weaknesses and didn't find any on a first pass. Of course, it is easy to cause QhullError more broadly when generators aren't in general position (coplanarity, degeneracy, etc.).

rtshort and others added 8 commits Jan 27, 2019
* minor adjustments to the new ConvexHull
visibility unit tests added in test_qhull.py
so that they no longer raise a NameError

* remove unrelated changes in Delaunay &
Voronoi to keep this PR focused on the primary
new feature -- visibility of ConvexHull facets

* fix refguide errors for changes in the
ConvexHull docstring

* fix pycodestyle errors in new ConvexHull
visibility unit tests

* add support for incremental visibility updates
in ConvexHull

* add several unit tests, including those related to
incremental visibility and accessing hull.good in absence
of QG options
@tylerjereddy

This comment has been minimized.

Copy link
Contributor

tylerjereddy commented Mar 23, 2019

The pypy3 failure looks like a connection / 404 issue that is completely unrelated; pypy3 CI testing is frequently problematic.

The rest of the CI is green after my fresh rebase this morning, I've given two weeks grace for additional review comments / issues, but no objections and looks ok to me after making a few adjustments, so in it goes. Thanks @rtshort

I've updated the release notes at https://github.com/scipy/scipy/wiki/Release-note-entries-for-SciPy-1.3.0

@tylerjereddy tylerjereddy merged commit b4e9b64 into scipy:master Mar 23, 2019
9 of 10 checks passed
9 of 10 checks passed
ci/circleci: pypy3 Your tests failed on CircleCI
Details
ci/circleci: build_docs Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scipy.scipy Build #20190323.1 succeeded
Details
scipy.scipy (Linux_Python_36_32bit_full) Linux_Python_36_32bit_full succeeded
Details
scipy.scipy (Windows Python35-64bit-full) Windows Python35-64bit-full succeeded
Details
scipy.scipy (Windows Python36-32bit-full) Windows Python36-32bit-full succeeded
Details
scipy.scipy (Windows Python36-64bit-full) Windows Python36-64bit-full succeeded
Details
scipy.scipy (Windows Python37-64bit-full) Windows Python37-64bit-full succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.