Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add arrow_data_to_origin option #217

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add arrow_data_to_origin option #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ff7bcb
add arrow_data_to_origin option
j042 Sep 15, 2024
d6ceb0a
doc: small addition to examples titles for easier understanding of wh…
clemens-fricke Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions examples/show_faces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,19 @@
)

# show
tri.show()
quad.show()
gus.show(["triangles", tri], ["quads", quad])

# plot data - plots vector data as arrows
for mesh in [tri, quad]:
mesh.vertex_data["coords"] = np.random.default_rng().random(
tri.vertices.shape
)
Comment on lines +65 to +67
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

The code segment that assigns random coordinates to the coords attribute of the vertex_data dictionary for each mesh is correct and contributes to the dynamic visualization of the mesh data.

To improve the code readability, consider extracting the random coordinate generation logic into a separate function or variable. For example:

def generate_random_coords(shape):
    return np.random.default_rng().random(shape)

for mesh in [tri, quad]:
    mesh.vertex_data["coords"] = generate_random_coords(mesh.vertices.shape)

This change is not necessary but can make the code more readable and maintainable.

mesh.show_options(arrow_data="coords")
gus.show(["triangles with arrows", tri], ["quads with arrows", quad])

# point data to origin
for mesh in [tri, quad]:
mesh.show_options(arrow_data_to_origin=True)
gus.show(
["triangles arrows to origin", tri], ["quads arrows to origin", quad]
)
clemens-fricke marked this conversation as resolved.
Show resolved Hide resolved
9 changes: 8 additions & 1 deletion gustaf/helpers/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def __init__(self, key, default):
"dict with following items are accepted: "
"{title: str, pos: tuple, size: list, title_font: str, "
"title_xoffset: float, title_yoffset: float, title_size: float, "
"title_rotation: float, nlabels: int, label_font:str, "
"title_rotation: float, nlabels: int, label_font: str, "
"label_size: float, label_offset: float, label_rotation: int, "
"label_format: str, draw_box: bool, above_text: str, below_text: str, "
"nan_text: str, categories: list}",
Expand All @@ -180,6 +180,13 @@ def __init__(self, key, default):
"cmap, colors are based on the size of the arrows.",
(str, tuple, list, int),
),
Option(
"vedo",
"arrow_data_to_origin",
"Points arrow data to geometric origin."
"Equivalent to shifting arrows backwards with their own magitudes.",
(bool,),
),
Comment on lines +183 to +189
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Fix typo and enhance documentation.

The implementation looks good and aligns well with the PR objectives. However, there are a few minor improvements needed:

  1. Fix the typo "magitudes" → "magnitudes"
  2. Consider enhancing the description to clarify:
    • The default behavior (arrows point away from origin)
    • The relationship with other arrow options (arrow_data and arrow_data_scale)

Here's the suggested improvement:

     Option(
         "vedo",
         "arrow_data_to_origin",
-        "Points arrow data to geometric origin."
-        "Equivalent to shifting arrows backwards with their own magitudes.",
+        "Points arrow data to geometric origin. By default, arrows point away from origin. "
+        "When enabled, arrows are shifted backwards by their own magnitudes. "
+        "Works in conjunction with arrow_data and arrow_data_scale options.",
         (bool,),
     ),
📝 Committable 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.

Suggested change
Option(
"vedo",
"arrow_data_to_origin",
"Points arrow data to geometric origin."
"Equivalent to shifting arrows backwards with their own magitudes.",
(bool,),
),
Option(
"vedo",
"arrow_data_to_origin",
"Points arrow data to geometric origin. By default, arrows point away from origin. "
"When enabled, arrows are shifted backwards by their own magnitudes. "
"Works in conjunction with arrow_data and arrow_data_scale options.",
(bool,),
),

Option(
"vedo",
"axes",
Expand Down
15 changes: 13 additions & 2 deletions gustaf/show.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,10 +367,11 @@ def make_showable(obj, as_dict=False, **kwargs):

# we are here because this data is not a scalar
# is showable?
if arrow_data_value.shape[1] not in (2, 3):
a_data_dim = arrow_data_value.shape[1]
if a_data_dim not in (2, 3):
raise ValueError(
"Only 2D or 3D data can be shown.",
f"Requested data is {arrow_data_value.shape[1]}",
f"Requested data is {a_data_dim}D.",
Comment on lines +371 to +374
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Combine Error Messages into a Single String

When raising the ValueError, it's better to combine the error messages into a single string to ensure that the complete message is displayed correctly.

Apply this diff to combine the error message:

if a_data_dim not in (2, 3):
    raise ValueError(
-        "Only 2D or 3D data can be shown.",
-        f"Requested data is {a_data_dim}D.",
+        f"Only 2D or 3D data can be shown. Requested data is {a_data_dim}D."
    )
📝 Committable 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.

Suggested change
if a_data_dim not in (2, 3):
raise ValueError(
"Only 2D or 3D data can be shown.",
f"Requested data is {arrow_data_value.shape[1]}",
f"Requested data is {a_data_dim}D.",
if a_data_dim not in (2, 3):
raise ValueError(
f"Only 2D or 3D data can be shown. Requested data is {a_data_dim}D."
)

)

as_edges = from_data(
Expand All @@ -379,6 +380,16 @@ def make_showable(obj, as_dict=False, **kwargs):
obj.show_options.get("arrow_data_scale", None),
data_norm=obj.vertex_data.as_scalar(arrow_data),
)

if obj.show_options.get("arrow_data_to_origin", False):
# point arrow to the origin instead
arrow_shift = np.diff(
as_edges.vertices.reshape(-1, 2, a_data_dim), axis=1
)
as_edges.vertices[:] = (
as_edges.vertices.reshape(-1, 2, a_data_dim) - arrow_shift
).reshape(-1, a_data_dim)
Comment on lines +386 to +391
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor Arrow Adjustment Code for Clarity

Consider refactoring the code that adjusts the arrow vertices to improve readability and efficiency.

Apply this diff to simplify the code:

-        arrow_shift = np.diff(
-            as_edges.vertices.reshape(-1, 2, a_data_dim), axis=1
-        )
-        as_edges.vertices[:] = (
-            as_edges.vertices.reshape(-1, 2, a_data_dim) - arrow_shift
-        ).reshape(-1, a_data_dim)
+        arrow_vertices = as_edges.vertices.reshape(-1, 2, a_data_dim)
+        arrow_shift = np.diff(arrow_vertices, axis=1)
+        arrow_vertices -= arrow_shift
+        as_edges.vertices = arrow_vertices.reshape(-1, a_data_dim)

This refactoring reduces the number of reshapes and makes the code easier to understand.

📝 Committable 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.

Suggested change
arrow_shift = np.diff(
as_edges.vertices.reshape(-1, 2, a_data_dim), axis=1
)
as_edges.vertices[:] = (
as_edges.vertices.reshape(-1, 2, a_data_dim) - arrow_shift
).reshape(-1, a_data_dim)
arrow_vertices = as_edges.vertices.reshape(-1, 2, a_data_dim)
arrow_shift = np.diff(arrow_vertices, axis=1)
arrow_vertices -= arrow_shift
as_edges.vertices = arrow_vertices.reshape(-1, a_data_dim)


arrows = vedo.Arrows(
as_edges.vertices[as_edges.edges],
c=obj.show_options.get("arrow_data_color", "plasma"),
Expand Down
Loading