search

JuhaHeiskala / DirectQhull.jl

MIT License
12 stars 5 forks source link

0- vs 1-based indexing #7

Closed thchr closed 1 year ago

thchr commented 1 year ago

This may be intentional, but presently the indexes in e.g. .regions and .point_region are 0-based rather than the 1-based used in Julia. It might be nice to manually convert the output from qhull to 1-based indexes, in order to ensure that things like v.vertices[v.regions[x]] work as intended?

thchr commented 1 year ago

AFAICT, the .simplices of ConvexHull do seem to be 1-based, so I suppose it is mostly a matter of making sure the various indexes are returned in a consistent 1-based indexing?

JuhaHeiskala commented 1 year ago

Not intentional, .regions and .point_region being 0-based is due to copying from scipy code and neglecting to account for Python's 0-based indexes. Actually looked like that there might be a couple of other bugs also due to qh_pointid returning 0-based index for the point and that not being accounted for when accessing Julia vectors. I will make a fix for these.

JuhaHeiskala commented 1 year ago

For the .regions field the 0-value indicates the vertex at infinity, if I understood qhull correctly. So this v.vertices[v.regions[x]]would still need some way to handle the point at infinity.

thchr commented 1 year ago

So this v.vertices[v.regions[x]]would still need some way to handle the point at infinity.

Personally, I think it's fine that the point at infinity corresponds to index 0 (-1 in qhull, right?): the caller would just have to check if the index is zero before attempting to index into v.vertices.

JuhaHeiskala commented 1 year ago

ok, lets leave it as it is now. I think it is 0 is vertex at infinity inside qhull, scipy translated that to -1, so that 0-based indexing python arrays works.