search

tylerjereddy / scipy

Scipy library main repository
http://scipy.org/scipylib/
BSD 3-Clause "New" or "Revised" License
1 stars 1 forks source link

Returned data structues #5

Closed niknow closed 9 years ago

niknow commented 9 years ago

Hey Tyler,

you indicated that you are not completely happy with the data structues returned by the SphericalVoronoi class. In my last commit 33bf2255aeabf2e75745835bdf416e16664914f2, I added two properties to this class, namely vertices and regions. They are calculated in addition in calc_vertices. With these modifications, the attributes

SphericalVoronoi.points SphericalVoronoi.vertices SphericalVoronoi.regions

should behave analogously to the

Voronoi.points Voronoi.vertices Voronoi.regions

from scipy.spatial.Voronoi as documented here:

http://docs.scipy.org/doc/scipy-dev/reference/generated/scipy.spatial.Voronoi.html

This needs further testing, at least one unittest and maybe a bit of streamlining though. If you are happy, I can proceed with that.

tylerjereddy commented 9 years ago

Yeah, that sounds sensible. I think they will eventually ask us to use that data structure for unification with the other things in scipy.spatial

tylerjereddy commented 9 years ago

Something is wrong with the unit tests though--did you ensure that python runtests.py -v runs to completion for each push? The tests stop on my machine when they get to the spherical Voronoi ones, which means something has changed there vs. previous behaviour.

tylerjereddy commented 9 years ago

Although the unit tests do run to completion eventually, they are noticeably slower than they should be as of commit 33bf2255aeabf2e75745835bdf416e16664914f2 -- I'll open a separate issue for this (#6).

tylerjereddy commented 9 years ago

I suspect what is happening is that there are redundant calculations going on to provide the separate data structures? You do mention that they are performed in addition to the original calculations. We'll want to gracefully replace the original data structures and adjust all tests cases and docs accordingly, while verifying that the quadratic time complexity performance curves remain unchanged.

tylerjereddy commented 9 years ago

Also, is self.regions returning the regions with vertices properly sorted? It seems to be assigned before the sorting happens.

niknow commented 9 years ago

At the moment the self.regions and self.vertices is calculated in addition, so it's reasonable that this takes a bit longer.

I left the original calculation in place to not break the unittests. ;) I tested it seperately against an existing scipy build, but will look into it.

The self.regions is not returning the regions with sorted vertices. I think scipy.spatial.Voronoi.regions isn't doing that either (?) because depending on what the user of the library wants to do with the outcome, this might not be necessary, and producing the result in this form is quite efficient.

However, I think we should try to simplify the _sort_vertices function to produce the sorted result using the unsorted result. This should in sum not take much longer and then users who want this sorted result (for instance to pass it into Poly3DCollection) can still do that and unittests relying on this interface should still work.

tylerjereddy commented 9 years ago

Ok, and the code / tests are now python 2.x / 3.x compliant as well, so we should try to sustain that moving forward (shouldn't be difficult).