search

QInfer / python-qinfer

Library for Bayesian inference via sequential Monte Carlo for quantum parameter estimation.
BSD 3-Clause "New" or "Revised" License
92 stars 31 forks source link

Region estimation troubles #83

Closed ihincks closed 8 years ago

ihincks commented 8 years ago

This applies to the two methods region_est_hull and regioun_est_ellipsoid in SMCUpdater.

I get a generic Qhull error when I try to do it.

If you check out the documentation for Delaunay triangulation, you can see there is some kind of switch for ndim>4 in terms of passing arguments to the qhull C++ library.

ihincks commented 8 years ago

Ah, false alarm, this turned out to be my problem. My fifth model parameter happened to be almost exactly 0 for every particle and Dalaunay triangulation doesn't like this. Confusingly, this dimension just happened to coincide with the argument switch in qhull.

ihincks commented 8 years ago

Different but related (and hopefully real) problem: SMCUpdater.region_est_hull seems to assume 3 dimensions. In particular, in the lines 

for ia, ib, ic in hull:
    faces.append(points[[ia, ib, ic]])

hull is a list of indices of the original points specifying the exterior simplices of the convex hull. For example, if there are 3 dimensions (model parameters), then the external simplices will be triangles, and if there are 4 dimensions, the external simplices will be tetrahedra, etc. In this snippet, ia, ib, ic therefore correspond to which original points the corners of the triangles on the exterior of the convex hull are. If the number of model parameters is different than 3, this doesn't make sense. (This is easy to fix, just want to make sure I understand).

Moreover, there is a warning "Computing convex hulls via the Delaunay triangulation is inefficient and subject to increased numerical instability. Use ConvexHull instead." on the doc page. So I propose moving to ConvexHull, which should be easy and do the same thing.

csferrie commented 8 years ago

Yes, this is a real issue. I only implemented it for some plotting purposes on qubit states. If it works for more than 3 dimensions, that would be quite nice.

cgranade commented 8 years ago

I think that the scipy.spatial.ConvexHull class is new as of late 2013, right after @csferrie originally added the region_est_hull method. We'll take a crack at moving things over to the new class, then, as I think that makes a lot of sense to use.

ihincks commented 8 years ago

I'm almost done the required changes, here, will submit a PR sometime.

cgranade commented 8 years ago

Ah, I didn't realize, sorry. I'm guessing #87 is redundant then, sorry.

On Thu, Sep 8, 2016, 21:42 Ian Hincks notifications@github.com wrote:

I'm almost done the required changes, here, will submit a PR sometime.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/QInfer/python-qinfer/issues/83#issuecomment-245571875, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB7HDbsjfRKMHz1KTHFrcODV5Xj6sViks5qn_SNgaJpZM4J3ILS .

ihincks commented 8 years ago

Oh, you are so quick :). So I also added param_slice, but I added it to all three methods in that section. I also added in_credible_region (pun intended) that works with both the convex hull and the enclosing ellipsoid. So I think our stuff will be compatible. Maybe I will just PR to your branch or something.

cgranade commented 8 years ago

Resolved by merging #84 and #86.