Support for 3d subset and a lasso 3d selection #1522
Conversation
There was a problem hiding this comment.
@maartenbreddels - thanks, this is great! I just tested it out with the VisPy 3d scatter plot (which I edited to use this locally) and it works great!
One thing I have in the 3D viewer which we might want to consider porting here is to make the selection operate in chunks for large datasets, to avoid creating large temporary arrays during the Matrix multiplication. However, I think what might be even better would be to write a multi-threaded C/Cython extension that implements the various contains methods for the 2D and projected 3D ROIs, though that can be a project for another time :) (unless you know of something like this existing already?)
glue/core/roi.py
Outdated
| @@ -561,6 +575,44 @@ def move_to(self, xdelta, ydelta): | |||
| self.vy = list(map(lambda y: y + ydelta, self.vy)) | |||
|
|
|||
|
|
|||
| class PolygonalProjected3dROI(PolygonalROI): | |||
There was a problem hiding this comment.
Could you add projected 3D ROIs for rectangle and circle too in a similar way?
glue/core/roi.py
Outdated
| :returns: A Boolean array, where each element is True | ||
| if the corresponding (x,y,z) tuple is inside the Roi. | ||
|
|
||
| :raises: UndefinedROI exception if not defined |
There was a problem hiding this comment.
Could you use Numpydoc format here? I'm trying to transition the docstrings to numpydoc progressively, so it would be good to avoid adding new code that's not in Numpydoc.
glue/core/roi.py
Outdated
| screen_h = np.array(np.dot(self.projection_matrix, vertices)) | ||
| # convert to screen coordinates, and we don't care about z | ||
| x, y = screen_h[:2] / screen_h[3] | ||
| return self.contains(x, y) |
There was a problem hiding this comment.
The code here doesn't work if x, y, and z are not 1D - it would be nice if we allowed x, y and z to be n-dimensional arrays and returned an n-dimensional mark at the end (I guess the easiest might be to unravel)
glue/core/roi.py
Outdated
| # projections as well | ||
| vertices = np.array([x, y, z, np.ones(len(x))]) | ||
| # homogeneous screen coordinates | ||
| screen_h = np.array(np.dot(self.projection_matrix, vertices)) |
There was a problem hiding this comment.
I think np.matmul is now the recommended matrix multiplication function
There was a problem hiding this comment.
That also didn't do it, but tensordot seemed more appropriate without needing to do transposes.
glue/core/subset.py
Outdated
| y = data[self.yatt, view] | ||
| z = data[self.zatt, view] | ||
|
|
||
| # if (x.ndim == data.ndim and |
There was a problem hiding this comment.
I think you can basically ignore/remove this - it was an optimization that was useful when selecting a 2D region in an n-d cube in the image viewer, but since the 3D viewers don't (yet) support showing n-dimensional datasets, this isn't useful. The only thing you need to properly support cubes is to make contains3d work with n-dimensional arrays.
glue/core/roi.py
Outdated
| y = np.asarray(y) | ||
| # work in homogeneous coordinates so we can support perspective | ||
| # projections as well | ||
| vertices = np.array([x, y, z, np.ones(len(x))]) |
|
I forgot to mention, it would be good to add some simple tests of this, at least to make sure the code runs. |
|
Yes, I didn't want to do unittests before you saw it, but I'll get to it next week. In vaex this would be multithreaded and sequential (good for memory mapping). With dask the sequential isn't an option I think. |
Would it be worth splitting out the multi-threaded selection code into a lightweight standalone library that isn't tied to vaex? What I'm thinking of is a library where the main API is simply a set of |
glue/core/roi.py
Outdated
| ---------- | ||
|
|
||
|
|
||
| x : ndarray |
There was a problem hiding this comment.
Can you get rid of the empty lines on line 82 and 83?
There was a problem hiding this comment.
Can you replace ndarray by:
:class:`numpy.ndarray`
Some older docstrings may just have ndarray but this doesn't create an intersphinx link
glue/core/roi.py
Outdated
| Returns | ||
| ------- | ||
| ndarray | ||
| A Boolean array, where each element is True if the corresponding |
glue/core/roi.py
Outdated
| def _project(projection_matrix, x, y, z): | ||
| """Projects 3d coordinates to 2d coordinates using a 4x4 matrix""" | ||
| if not isinstance(x, np.ndarray): | ||
| x = np.asarray(x) |
There was a problem hiding this comment.
I think you can get rid of if not isinstance(x, np.ndarray): as I think np.asarray doesn't do a copy if the input is already an array?
glue/core/roi.py
Outdated
| class Projected3dROI(Roi): | ||
| """"A region of interest defined in screen coordinates. | ||
|
|
||
| The screen coordiantes are defined by the projection matrix. |
|
Ok, thanks! I guess you squash this all, or do you want me to squash it in >1 commit? |
|
@maartenbreddels - no squashing required :) |
astrofrog
changed the title
|
The Travis failures are unrelated - thanks! |
Not so sure about what to do with the to_mask for image data. But if it looks good I can take a look at adding unittests and that may clarify things.