Create mesh visualization #184
Conversation
WalkthroughThe recent changes introduce a new function, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
Re-assign me when the CI debacle is resolved and I'll take a look. I can say I'm definitely a fan of the end result though! 🚀 |
To trigger the CI just open a pull request directly on main #185 |
…retoolkit into create_mesh_visualization
|
I got the feeling that I should make an example notebook for this PR. What do you think? |
There was a problem hiding this comment.
No major issues, just a bunch of nits from my end.
A bunch of the inputs take a None value; in this case it would be nice to know what the default behaviour is. I see that these basically all passed directly on to plotly.graph_object.Isosurface. From my perspective, it would be sufficient to then have something like "cf. plotly.graph_object.Isosurface docs for default behaviour on optional input". The only thing I don't like about that is that when I go to the docs for that class (sorry, link is just to the whole module page, I don't seem to be able to get a section link...), I don't see any explanations there either! This is not your fault, but if you know where to find an actual description of these parameters, it might be worth linking it...
Finally, would it make sense to directly expose something like **plotly_isosurface_kwargs and pass these on directly, trusting the user to not screw things up if they make these kwargs non-empty?
| value: (numpy.ndarray): Value to plot. Must have a shape of (nx, ny, nz) | ||
| cell (Atoms|ndarray|list|float|tuple): Cell, ignored if | ||
| `structure_plot` is given | ||
| structure_plot (plotly.graph_objs._figure.Figure): Plot of the |
There was a problem hiding this comment.
I guess the type hint should show import directly from the public part of the API (https://plotly.com/python-api-reference/generated/plotly.graph_objects.Figure.html) and not ..._figure.Figure
| surface_fill(float): Polygonal filling of the surface to choose between | ||
| 0 and 1 | ||
| opacity(float): Opacity | ||
| surface_count(int): Number of isosurfaces, 5 by default, which means |
There was a problem hiding this comment.
I believe you...but why? I would have thought it was 2 just like it was until the last commit (at time of writing). This is so weird it might warrant some sort of "I know, I know, but seriously, it's because X" in the docstring.
There was a problem hiding this comment.
yeah I thought about it a little bit XD The problem is that somehow plotly sets the two layers for the highest value and the lowest value. It makes only sense if there are multiple points with the max/min value, but that's rarely the case. So even with 5, in reality you would see only 3 in the middle. I also thought about changing isomax and isomin, but that felt a bit like too much manipulation.
There was a problem hiding this comment.
That's very fair. Maybe something like , which actually means only min and max because of details of how plotly handles the levels to explain the weirdness just a little?
Co-authored-by: Liam Huber <[email protected]>
Co-authored-by: Liam Huber <[email protected]>
There was a problem hiding this comment.
With doing the changes/additions in the docstring suggested by @liamhuber , this PR is perfectly fine to merge from my side. Only thing is that I guess usage is a bit obscure to a user without prior knowledge, so I think that you really should add an example notebook (as you already suggested earlier).
…esh_visualization # Conflicts: # structuretoolkit/visualize.py
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (2)
structuretoolkit/__init__.py (1)
97-97: Ensure usage of imported functions.The
plot_isosurfacefunction is imported but not used within the codebase. Ensure that it is utilized appropriately or added to__all__if intended for public API access.
structuretoolkit/__init__.py: Import statement at line 97.Analysis chain
Ensure usage of imported functions.
The
plot_isosurfacefunction is imported but not used within this file. Ensure that it is utilized appropriately or added to__all__if intended for public API access.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `plot_isosurface` within the codebase. # Test: Search for the function usage. Expect: At least one occurrence of the function usage. rg --type python -A 5 $'plot_isosurface'Length of output: 70
Script:
#!/bin/bash # Description: Verify usage of `plot_isosurface` within the codebase. # Test: Search for the function usage. Expect: At least one occurrence of the function usage. rg 'plot_isosurface'Length of output: 163
Tools
Ruff
97-97:
structuretoolkit.visualize.plot3dimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
97-97:
structuretoolkit.visualize.plot_isosurfaceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
structuretoolkit/visualize.py (1)
831-857: Enhance function documentation.The function docstring is clear but can benefit from additional details and formatting improvements.
""" Make a mesh plot Args: mesh (numpy.ndarray): Mesh grid. Must have a shape of (3, nx, ny, nz). It can be generated from structuretoolkit.create_mesh. value (numpy.ndarray): Value to plot. Must have a shape of (nx, ny, nz). cell (Atoms|ndarray|list|float|tuple): Cell, ignored if `structure_plot` is given. structure_plot (plotly.graph_objs._figure.Figure): Plot of the structure to overlay. You should basically always use structuretoolkit.plot3d(structure, mode="plotly"). isomin (float): Min color value. isomax (float): Max color value. surface_fill (float): Polygonal filling of the surface to choose between 0 and 1. opacity (float): Opacity. surface_count (int): Number of isosurfaces, 5 by default, which means only min and max. colorbar_nticks (int): Colorbar ticks correspond to isosurface values. caps (dict): Whether to set cap on sides or not. Default is False. You can set: caps=dict(x_show=True, y_show=True, z_show=True). colorscale (str): Colorscale ("turbo", "gnbu" etc.). height (float): Height of the figure. 600px by default. camera (str): Camera perspective to choose from "orthographic" and "perspective". Default is "orthographic". Returns: plotly.graph_objs._figure.Figure: The plotly figure object. """
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- structuretoolkit/init.py (1 hunks)
- structuretoolkit/visualize.py (2 hunks)
Additional context used
Ruff
structuretoolkit/__init__.py
97-97:
structuretoolkit.visualize.plot3dimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
97-97:
structuretoolkit.visualize.plot_isosurfaceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
structuretoolkit/visualize.py
817-817: Undefined name
Union(F821)
818-818: Undefined name
plotly(F821)
825-825: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
861-861: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Additional comments not posted (1)
structuretoolkit/visualize.py (1)
5-5: LGTM!The import statement for future annotations is correct and follows best practices.
| opacity: Optional[float] = None, | ||
| surface_count: int = 5, | ||
| colorbar_nticks: Optional[int] = None, | ||
| caps: Optional[dict] = dict(x_show=False, y_show=False, z_show=False), |
There was a problem hiding this comment.
Avoid mutable default arguments.
Using mutable default arguments can lead to unexpected behavior. Replace with None and initialize within the function.
- caps: Optional[dict] = dict(x_show=False, y_show=False, z_show=False),
+ caps: Optional[dict] = None,Initialize within the function:
if caps is None:
caps = dict(x_show=False, y_show=False, z_show=False)Tools
Ruff
825-825: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
| try: | ||
| import plotly.graph_objects as go | ||
| except ModuleNotFoundError: | ||
| raise ModuleNotFoundError("plotly not installed") |
There was a problem hiding this comment.
Improve exception handling.
Use raise ... from None to distinguish exceptions from errors in exception handling.
- raise ModuleNotFoundError("plotly not installed")
+ raise ModuleNotFoundError("plotly not installed") from NoneCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise ModuleNotFoundError("plotly not installed") | |
| raise ModuleNotFoundError("plotly not installed") from None |
Tools
Ruff
861-861: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
| @@ -807,3 +809,78 @@ def _get_flattened_orientation( | |||
| flattened_orientation[:3, :3] = _get_orientation(view_plane) | |||
|
|
|||
| return (distance_from_camera * flattened_orientation).ravel().tolist() | |||
|
|
|||
|
|
|||
| def plot_isosurface( | |||
There was a problem hiding this comment.
Add missing import for Union.
The Union type hint is used but not imported. Import it from the typing module.
+from typing import UnionCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def plot_isosurface( | |
| +from typing import Union | |
| def plot_isosurface( |
| mesh, | ||
| value, | ||
| cell: Optional[Union[Atoms, list, np.ndarray, float]] = None, | ||
| structure_plot: Optional["plotly.graph_objs._figure.Figure"] = None, |
There was a problem hiding this comment.
Add missing import for plotly.
The plotly module is referenced in type hints but not imported. Import it to avoid undefined name errors.
+import plotly.graph_objects as goCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| structure_plot: Optional["plotly.graph_objs._figure.Figure"] = None, | |
| import plotly.graph_objects as go | |
| structure_plot: Optional["plotly.graph_objs._figure.Figure"] = None, |
Tools
Ruff
818-818: Undefined name
plotly(F821)
Example:
Output:
Summary by CodeRabbit
plot_isosurfacefunction for creating detailed mesh plots usingplotly.