Scale array for points gaussian #4366
Conversation
pyvista/plotting/plotting.py
Outdated
| if scale is not None and style == 'points_gaussian': | ||
| if isinstance(scale, str): | ||
| self.mapper.scale_array = scale | ||
| self.mapper.emissive = emissive | ||
| self.mapper.scale_factor = 1.0 | ||
|
|
||
|
|
There was a problem hiding this comment.
I'm weary of this block. Specifically, I'm weary against having the scale argument specifically for the point gaussian mapper and silently ignored for all other scenarios.
Further, if scale is a number, should we set scale_factor with that value?
Additionally, I'm pretty sure it is unnecessary to set the emissive here as it should be set in the PointGaussianMapper instantiation on line ~3217 (~3198 head)
There was a problem hiding this comment.
Thank you for your comments!
- Indeed
scaleis only used with the point gaussian mapper here (likeemissivefor example) so I'm creating a new condition here because in this case I need to execute the code from the condition withrender_points_as_spheresto runuse_circular_splat. And I didn't want to move too much code. - For
scale_factor, do you mean thatscalecould also be a number and work the way you describe? - About
emissive, I forgot to explain this, it is set toTruein the definition ofuse_circular_splat, maybe it should be done differently here?
There was a problem hiding this comment.
I'm also +1 on the functionality here being added to PyVista, I think the discussion is around what user interface to this should look like.
I am already confused by the interaction and overlap of all the options in add_mesh, e.g. when does point_size matter? How will this be different than scale in this PR? Do these interact? There are render_points_as_spheres=True/False and style=point_guassian. I believe the former does something different when the style changes. IMO the problem will continue to grow unless we address this as new options are added.
There was a problem hiding this comment.
I think what we're seeing is "feature creep" in the form of adding parameter after parameter to add_mesh.
I think the original implementation is flawed, and @kmarchais is just following our design patten of adding another kwarg to add_mesh.
How about this: add_points_gaussian. That's explicit and permits us to trim down on args in the already bloaded add_mesh.
There was a problem hiding this comment.
I agree with @akaszynski that we need to rethink the design pattern. One path forward for this PR is to align on a name for this parameter, and then break it out into a separate method in the future, unless @kmarchais wants to take this part on. In this case, it would be okay if scale is ignored in all other cases.
There was a problem hiding this comment.
Let's add add this for now in this PR and as a follow up implement add_points_gaussian. The only caveat will be that we shouldn't include this in the upcoming release this week, as awesome as this example is.
There was a problem hiding this comment.
Yes, it still requires some more improvement such as dealing with different types in a similar way as it's done with the scale parameter for glyphs. Also applying emissive here is probably not the best way, but probably not in use_circular_splat neither.
There was a problem hiding this comment.
I have vague memories of someone planning to reduce the number of add_*() methods. It may have been @banesullivan? I'm not at all sure I remember correctly, and I can't find any related discussion, so just pointing this out because in case I remember correctly, it's better to short-circuit contradictory fundamental design plans like this.
There was a problem hiding this comment.
That was me, @adeak -- indeed, I want to reduce the number of add_*() methods on the plotter, but mostly in regards to special handling of dataset/mesh types whereas this use case is for special handling of rendering techniques, so I think an add_point_gaussian()-like method would be acceptable. Method like add_lines() and add_points() need to go -- these should be handled by add_mesh(). I have a more thorough explanation of my rationale in some comments somewhere.
+1 for creating a clean compromise to merge this PR for release, then rethinking the API design later.
The confliction between scale and point_size also concerns me... I'm tempted to call this an "advanced" feature and have users set it on the mapper specifically rather than as another kwarg in add_mesh(). With usage looking like:
actor = plotter.add_mesh(mesh, style='points_gaussian', ...)
actor.mapper.scale_array = 'name of array'There was a problem hiding this comment.
@banesullivan, I've implemented your suggestion.
All, please verify that this API works. I think it balances everyone's requests.
There was a problem hiding this comment.
This is an awesome addition and an excellent example! Thanks for the contribution, @kmarchais!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4366 +/- ##
==========================================
+ Coverage 96.38% 96.40% +0.01%
==========================================
Files 134 134
Lines 22721 22727 +6
==========================================
+ Hits 21900 21909 +9
+ Misses 821 818 -3 |
|
@kmarchais, please check over the changes and let me know if you're fine with the new API: >>> import numpy as np
>>> import pyvista as pv
>>> n_spheres = 1_000
>>> pos = np.random.random((n_spheres, 3))
>>> rad = np.random.random(n_spheres) * 0.01
>>> pdata = pv.PolyData(pos)
>>> pdata['radius'] = rad
>>> pl = pv.Plotter()
>>> actor = pl.add_mesh(
... pdata,
... style='points_gaussian',
... emissive=False,
... render_points_as_spheres=True,
... )
>>> actor.mapper.scale_array = 'radius'
>>> pl.enable_anti_aliasing('ssaa')
>>> pl.show()
|
…marchais/pyvista into feat/pointgaussian-scale-array
|
Works perfectly for me! Also if possible, it would be nice to be able to pass the array ( |
I thought of this as well and I agree that it seems like a good idea. However, I'm wary of allowing two input types but returning only one output type. While this would be more convenient, it's likely this would lead to confusion since this is a property (getter/setter) and I'd prefer to avoid side effects. |
|
@kmarchais, thanks for fixing the test. I ran it locally and it looks like the image is correct. I also verified that the scale indeed works and realized that we should raise if the point data array does not exist. Otherwise, it looks like it just scales everything to 1. |
pyvista/plotting/mapper.py
Outdated
| >>> pl.show() | ||
|
|
||
| """ | ||
| self.scale_factor = 1.0 |
There was a problem hiding this comment.
VTK allows for both of these settings to be applied independently, and the default for scale_factor is already 1.0 in vtk, but I haven't chased down the default in the pyvista code. This is a very useful feature. For example, I have data from a simulation that has particle size in mm stored in point data as "diameter", but the mesh positions are in units of m. I would want to set the scale_factor to 0.001 and scale_array to "diameter" to render this correctly.
There was a problem hiding this comment.
Ah yes, this shouldn't be hard set here
There was a problem hiding this comment.
And this is in the getter? Seems like a mistake
There was a problem hiding this comment.
| self.scale_factor = 1.0 |
There was a problem hiding this comment.
The function use_circular_spat does self.scale_factor *= 1.5 so I wanted to cancel it at first without modifying the function, I think it must be done somewhere
There was a problem hiding this comment.
This is a bit of an interconnected rabbit hole.
This line seems mysterious to me:
pyvista/pyvista/plotting/plotting.py
Line 3505 in d9dd94e
Then there is this line, which as @kmarchais points out further modifies scale_factor if a different option is used:
pyvista/pyvista/plotting/plotting.py
Line 3512 in d9dd94e
There was a problem hiding this comment.
We don't really define what point_size units are in our documentation. I have not been able to use this new points-gaussian feature successfully and have kept using glyphs as it isn't clear how to get the right physical mesh units for rendering. I don't want to further complicate this PR which was intended to be a simple addition, however. But I'm not sure if there is a simple fix. Maybe it would be be best to keep the current state, i.e. hard code scale factor, but in the setter. Then raise an issue to disentangle this complexity in a more holistic PR?
|
As much as I'd like to get this out in v0.39.0, I feel this feature isn't quite complete. I agree with @MatthewFlamm, we really need to overhaul our point size documentation and implementation. For the moment, @kmarchais, I'm afraid you'll have to use the VTK API to scale these points (or this branch). |
|
@kmarchais, I'd like to have this in the next release (0.43), so please ping me when it's ready for review. |
|
@akaszynski I've just updated this pull request to incorporate the latest developments from the main branch, allowing me to use this feature. It's working as expected on my end, so I believe it's ready. However, considering our prior discussion regarding its compatibility with the point size implementation, I'm not sure if it fits perfectly with your earlier comments. |
The main issue we have is our current implementation is non-ideal. Point size documentation is lacking and changing this will likely be non-backwards compatible in some ways. I wasn't comfortable with including this on the last few releases since we didn't have time to let this sit on Provided we review and agree on the changes moving forward, I'd be happy implementing it provided this cleans up our implementation. |
There was a problem hiding this comment.
I don't think this test is sufficient. Our image regression comparison likely won't be able to pick up on changes to these small dots. Can you make a more visually distinct test?
There was a problem hiding this comment.
With a scaling of 0.05 instead of 0.01 in tests/plotting/test_plotting.py for sphere["z"] = sphere.points[:, 2] * 0.010 we get the following result. I'm not sure how to upload the new result though.
There was a problem hiding this comment.
Thanks for this! We'll handle updating the baseline image if you're not able.
Running the single test with the --reset_image_cache option will do it. I.e.:
pytest tests/plotting/test_plotting.py -k test_plot_points_gaussian_scale --reset_image_cache
|
Overall, this looks okay to me and it's a greatly needed feature. @kmarchais, I sincerely apologize that it's taken so long to merge this... thank you for your patience. I propose we merge this shortly after the @tkoyama010, can you help me to remember to merge this after the |
|
Copy. I'll schedule the merge to be done at the right time. |
|
#4366 (comment) still hasn't been resolved. I still do not like how |
|
Indeed, I just moved it into the setter. The scale factor is changed in the if style == 'points_gaussian':
self.mapper.scale_factor = prop.point_size * self.mapper.dataset.length / 1300
if not render_points_as_spheres and not self.mapper.emissive:
if prop.opacity >= 1.0:
prop.opacity = 0.9999 # otherwise, weird triangles
if render_points_as_spheres:
if style == 'points_gaussian':
self.mapper.use_circular_splat(prop.opacity)I don't fully comprehend this part but that's why I need to reset it to 1.0 to use |
|
github-actions preview |
There was a problem hiding this comment.
It's time to merge this! If any problems or issues arise from this change, we will hopefully catch them early in this next release cycle.
I made a few changes to finalize this:
- Improved the image regression test so that it's more clear what's expected
- Moved the example into the "Plotting" category
- Ensured this example would not be interactive in our generated documentation
- Minor docs changes
Overview
Following #2576, I propose this feature to be able to use a scale array to scale points gaussian by a scalars array.
It allows to display a large number of spheres with different sizes.
Details
scale_arraypropertyscaleparameter to add_mesh (can only be the name of the array as a string for now, it could beTrueto select the active array or the array itself)