Swept Surface #438
Swept Surface #438
Conversation
|
Nice, can you post some screenshots? |
Sure! Be aware that work is still in progress.
|
WalkthroughThe recent updates introduce a new example script for creating and visualizing swept surfaces and solids using spline representations in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExampleScript as show_swept.py
participant SplinePy as splinepy.create.py
participant Visualizer as gustaf
User->>ExampleScript: Run script
ExampleScript->>SplinePy: Call swept(cross_section, trajectory)
SplinePy-->>ExampleScript: Return swept_surface
ExampleScript->>Visualizer: Visualize trajectory, cross-section, and swept_surface
ExampleScript->>User: Display result
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (2 hunks)
Files skipped from review due to trivial changes (1)
- examples/show_swept.py
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (1)
examples/show_swept.py (1)
1-4: Approve the import statements.The imports are correctly placed and necessary for the functionality demonstrated in this example.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (3)
- examples/show_swept.py (1 hunks)
- splinepy/bspline.py (2 hunks)
- splinepy/helpme/create.py (1 hunks)
Files not summarized due to errors (3)
- splinepy/helpme/create.py: Error: Server error. Please try again later.
- examples/show_swept.py: Error: Server error. Please try again later.
- splinepy/bspline.py: Error: Server error. Please try again later.
Additional comments not posted (5)
examples/show_swept.py (5)
1-5: Imports look good.The import statements for
gustaf,numpy, andsplinepyare necessary and correctly placed.
6-28: Trajectory definition is clear and correct.The trajectory is defined using a dictionary of parameters and is correctly initialized as a BSpline.
29-45: Cross-section definition is clear and correct.The cross-section is defined using a dictionary of parameters and is correctly initialized as a BSpline.
54-59: Swept surface creation is clear and correct.The swept surface is created using the
sweptfunction fromsplinepy.helpme.createwith the specified trajectory, cross-section, and normal vector.
61-71: Visualization section is clear and correct.The visualization section correctly uses
gustafto display the trajectory, cross-section, and swept surface with appropriate options.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (3)
- examples/show_swept.py (1 hunks)
- splinepy/bspline.py (2 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (17)
examples/show_swept.py (5)
1-2: Organize and validate import statements.Ensure that the imported modules are necessary and correctly used in the script. The import statements seem appropriate for the functionality in this script.
Also applies to: 4-4
8-27: Validate trajectory definition and initialization.The trajectory is defined using a dictionary and initialized as a B-spline. The trajectory is then uniformly refined. Ensure that the trajectory definition and refinement are correct and logical.
29-52: Validate cross-section definition and initialization.The cross-section is defined using a dictionary and initialized as a B-spline. The cross-section normal vector is also defined. Ensure that the cross-section definition and initialization are correct and logical.
54-59: Validate sweep operation.The sweep operation is performed using the
sweptfunction, which takes the trajectory, cross-section, and cross-section normal vector as inputs. Ensure that the sweep operation is correctly implemented.
61-71: Validate visualization.The resulting spline is visualized using the
gustaflibrary. Ensure that the visualization section is correctly implemented and provides a clear display of the spline structures.splinepy/bspline.py (5)
182-197: Method documentation is clear and detailed.The documentation for the
uniform_refine_fixed_knotsmethod is clear and provides detailed information about the parameters and return type.
199-204: Parameter handling is correct.The method correctly handles the
para_dimsparameter, ensuring it is a list of parametric dimensions to be refined.
206-228: Helper functiondetermine_new_knots_fixedis well-implemented.The helper function correctly calculates the new knots to be added between each pair of existing knots. The logic is sound and the implementation is efficient.
230-237: Knot insertion logic is correct.The method correctly determines the new knots for each parametric dimension and inserts them into the knot vector.
206-228: Helper functiondetermine_new_knots_fixedis well-implemented.The helper function correctly calculates the new knots to be added between each pair of existing knots. The logic is sound and the implementation is efficient.
splinepy/helpme/create.py (7)
301-307: Function documentation is clear and detailed.The documentation for the
sweptfunction is clear and provides detailed information about the parameters and return type.
332-340: Input validation is correct.The function correctly validates the input types and sets a default value for the
cross_section_normalparameter.
346-397: Transformation matrices calculation is correct.The function correctly calculates the transformation matrices for each trajectory point. The logic is sound and the implementation is efficient.
402-441: Knot insertion logic is correct.The function correctly calculates the curvature of the trajectory and inserts knots in areas with the highest curvature. The logic is sound and the implementation is efficient.
443-451: Cross-section translation is correct.The function correctly evaluates the center of the cross-section and translates it to the origin. The logic is sound and the implementation is efficient.
453-469: Control points transformation is correct.The function correctly transforms the cross-section control points along the trajectory. The logic is sound and the implementation is efficient.
475-504: Spline creation is correct.The function correctly creates the spline dictionary and checks if the spline is rational. The logic is sound and the implementation is efficient.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (3)
- examples/show_swept.py (1 hunks)
- splinepy/bspline.py (2 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (8)
examples/show_swept.py (5)
1-2: Organize imports for better readability.Consider grouping the imports into standard library imports, third-party imports, and local imports.
import numpy as np import gustaf as gus import splinepy
25-25: Remove commented-out code.The commented-out code on this line is unnecessary and can be removed to improve readability.
- # trajectory = splinepy.helpme.create.circle(10)
8-19: Consider adding a docstring for the trajectory definition.Adding a docstring will improve the readability and understanding of the code.
""" Define the trajectory as a B-spline with specified degrees, knot vectors, and control points. """ dict_trajectory = { "degrees": [2], "knot_vectors": [[0.0, 0.0, 0.0, 0.333, 0.666, 1.0, 1.0, 1.0]], "control_points": np.array( [ [0.0, 0.0, 0.0], [0.0, 0.0, 5.0], [10.0, 5.0, 0.0], [15.0, 0.0, -5.0], [20.0, 0.0, 0.0], ] ), }
30-42: Consider adding a docstring for the cross-section definition.Adding a docstring will improve the readability and understanding of the code.
""" Define the cross-section as a B-spline with specified degrees, knot vectors, and control points. """ dict_cross_section = { "degrees": [3], "knot_vectors": [[0.0, 0.0, 0.0, 0.0, 0.5, 1.0, 1.0, 1.0, 1.0]], "control_points": np.array( [ [0.0, 0.0, 0.0], [1.0, 2.0, 0.0], [2.0, 0.0, 0.0], [3.0, -2.0, 0.0], [4.0, 0.0, 0.0], ] ), }
50-52: Clarify the purpose of the normal vector definition.Adding a comment to explain the purpose of defining the normal vector will improve readability.
# Define the normal vector for the cross-section, used if the cross-section is not planar in the x-y plane. cs_nv = np.array([0, 0, 1])splinepy/helpme/create.py (3)
301-307: Add a docstring summary.The docstring should start with a brief summary of the function's purpose.
""" Sweeps a cross-section along a trajectory to create a swept surface. """
338-340: Inline comment for default value assignment.Adding an inline comment will improve readability.
if cross_section_normal is None: cross_section_normal = _np.array([0, 0, 1]) # Default normal vector
486-502: Add a comment to explain the rational check.Adding a comment to explain the rational check will improve readability.
# Check if the spline is rational and handle weights accordingly. if cross_section.is_rational or trajectory.is_rational: def weights(spline): if spline.is_rational: return spline.weights return _np.ones(spline.control_points.shape[0]) trajectory_weights = weights(trajectory) cross_section_weights = weights(cross_section) dict_swept_spline["weights"] = _np.outer(trajectory_weights, cross_section_weights).reshape(-1, 1) spline_type = _NURBS else: spline_type = _BSpline
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (7)
examples/show_swept.py (5)
1-7: Imports look good.The necessary modules for creating, visualizing, and exporting swept surfaces are imported.
93-116: Cross-section initialization looks good.The cross-section is correctly initialized as a B-spline.
139-182: Export functionality looks good.The swept surface is correctly exported as a mesh.
118-137: Swept surface creation and visualization look good.The swept surface is correctly created and visualized.
Ensure that the swept surface visualization options are correctly set.
Verification successful
Swept surface creation and visualization look good.
The swept surface is correctly created and visualized.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the swept surface visualization options. # Test: Search for the swept surface visualization options. Expect: Correctly set options. rg --type python 'swept_surface.show_options'Length of output: 76
Script:
#!/bin/bash # Description: Verify the swept surface visualization options. # Test: Search for the swept surface visualization options. Expect: Correctly set options. rg 'swept_surface.show_options' --glob '*.py'Length of output: 123
8-91: Verify thetrajectoryvariable usage.Ensure that the
trajectoryvariable is not shadowed by the commented lines to avoid confusion or errors in future modifications.splinepy/helpme/create.py (2)
330-342: Input type checks and default value settings look good.The input type checks and default value settings are correctly implemented.
306-328: Correct the typo in the documentation.Ensure that the parameter
cross_sectionin the documentation is corrected to match the function signature.- cross_section : Spline + cross_section: SplineLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (5)
splinepy/helpme/create.py (5)
301-305: Consider providing more detailed parameter descriptions.The function parameters are briefly described. Consider adding more details about their expected types and shapes.
356-358: Avoid recomputing the tangent vector.Store the tangent vector once to avoid recomputing it.
[PERFORMANCE]- x = traj.derivative([par_value[0]], [1]) - x = (x / _np.linalg.norm(x)).ravel() + x = (traj.derivative([par_value[0]], [1]) / _np.linalg.norm(traj.derivative([par_value[0]], [1]))).ravel()
375-395: Optimize the loop for transformation matrices computation.The loop for computing transformation matrices can be optimized by avoiding repeated calculations.
[PERFORMANCE]- for i in range(len(par_value)): - x = traj.derivative([par_value[i]], [1]) - x = (x / _np.linalg.norm(x)).ravel() - x_collection.append(x) - B.append(B[i] - _np.dot(B[i], x) * x) - B[i + 1] = B[i + 1] / _np.linalg.norm(B[i + 1]) - z = B[i + 1] - y = _np.cross(z, x) - T.append(_np.vstack((x, y, z))) - A.append(_np.linalg.inv(T[i])) + for i, param in enumerate(par_value): + x = (traj.derivative([param], [1]) / _np.linalg.norm(traj.derivative([param], [1]))).ravel() + x_collection.append(x) + B.append(B[i] - _np.dot(B[i], x) * x) + B[i + 1] = B[i + 1] / _np.linalg.norm(B[i + 1]) + T.append(_np.vstack((x, _np.cross(B[i + 1], x), B[i + 1]))) + A.append(_np.linalg.inv(T[-1]))
460-488: Optimize the curvature calculation and knot insertion logic.The curvature calculation and knot insertion logic can be optimized for better performance.
[PERFORMANCE]- curv = [] - for i in par_value: - curv.append(round(_np.linalg.norm(trajectory.derivative([i], [2])), 2)) - max_curv = max(curv) - max_indices = [i for i, x in enumerate(curv) if x == max_curv] - insertion_values = [] - par_values = par_value.ravel() - for maxi in max_indices: - if maxi == 0: - insertion_values.append((par_values[maxi] + par_values[maxi + 1]) / 2) - elif maxi == len(par_values) - 1: - insertion_values.append((par_values[maxi] + par_values[maxi - 1]) / 2) - else: - insertion_values.append((par_values[maxi] + par_values[maxi - 1]) / 2) - insertion_values.append((par_values[maxi] + par_values[maxi + 1]) / 2) + curv = _np.array([round(_np.linalg.norm(trajectory.derivative([i], [2])), 2) for i in par_value]) + max_curv = curv.max() + max_indices = _np.where(curv == max_curv)[0] + par_values = par_value.ravel() + insertion_values = _np.unique([(par_values[maxi] + par_values[maxi + 1]) / 2 if maxi == 0 else (par_values[maxi] + par_values[maxi - 1]) / 2 for maxi in max_indices])
503-543: Optimize the sweeping process.The sweeping process can be optimized by reducing redundant operations.
[PERFORMANCE]- evals = trajectory.evaluate(par_value) - cross_para_center = _np.mean(cross_section.parametric_bounds, axis=0) - cs_center = cross_section.evaluate(cross_para_center.reshape(-1, cross_section.para_dim)).ravel() - cross_section.control_points -= cs_center - A, R = transformation_matrices(trajectory, par_value) - swept_spline_cps = [] - for index, eval_point in enumerate(evals): - temp_csp = [] - for cscp in cross_section.control_points: - normal_cscp = _np.matmul(R, cscp) - normal_cscp = _np.matmul(A[index], normal_cscp) - normal_cscp += eval_point - temp_csp.append(normal_cscp) - swept_spline_cps.append(_np.array(temp_csp)) + evals = trajectory.evaluate(par_value) + cross_para_center = _np.mean(cross_section.parametric_bounds, axis=0) + cs_center = cross_section.evaluate(cross_para_center.reshape(-1, cross_section.para_dim)).ravel() + cross_section.control_points -= cs_center + A, R = transformation_matrices(trajectory, par_value) + swept_spline_cps = [_np.array([_np.matmul(A[index], _np.matmul(R, cscp)) + eval_point for cscp in cross_section.control_points]) for index, eval_point in enumerate(evals)]
|
@coderabbitai generate pytest-file for the sweeping routine |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (7)
examples/show_swept.py (5)
1-7: Imports look good!The import statements are correct and necessary for the functionality.
8-89: Trajectory definition is clear and logical.The trajectory is defined correctly using a dictionary and the
splinepy.BSplineclass. The commented-out alternative definitions provide useful examples.
97-116: Cross-section definition is clear and logical.The cross-section is defined correctly using the
splinepy.helpme.create.surface_circlefunction and is embedded into a NURBS spline.
122-143: Sweeping process is clear and logical.The sweeping process is defined correctly using the
splinepy.helpme.create.sweptfunction and visualization options are set appropriately.
144-187: Export process is clear and logical.The export process is defined correctly using the
splinepy.io.mfem.exportfunction to export the projection of the swept surface.splinepy/helpme/create.py (2)
301-306: Add type hints for function parameters and return type.Adding type hints improves code readability and helps with static analysis.
def swept( cross_section: Spline, trajectory: Spline, cross_section_normal: np.ndarray = None, auto_refinement: bool = False, ) -> Spline:
449-461: Optimize the rotation matrix computation.The rotation matrix computation can be optimized by using numpy functions directly.
- angle_of_cs_normal = _np.arctan2(cross_section_normal[2], cross_section_normal[0]) - R = _np.array([ - [_np.cos(angle_of_cs_normal), 0, _np.sin(angle_of_cs_normal)], - [0, 1, 0], - [_np.sin(angle_of_cs_normal), 0, _np.cos(angle_of_cs_normal)], - ]) - R = _np.where(_np.abs(R) < 1e-10, 0, R) + R = _np.eye(3) + angle_of_cs_normal = _np.arctan2(cross_section_normal[2], cross_section_normal[0]) + R[0, 0], R[0, 2] = _np.cos(angle_of_cs_normal), _np.sin(angle_of_cs_normal) + R[2, 0], R[2, 2] = -_np.sin(angle_of_cs_normal), _np.cos(angle_of_cs_normal)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- splinepy/helpme/create.py (1 hunks)
- tests/helpme/test_create.py (1 hunks)
Files not reviewed due to errors (1)
- splinepy/helpme/create.py (no review received)
Additional comments not posted (4)
tests/helpme/test_create.py (4)
348-365: Basic functionality test looks good.The test correctly checks if the result is not None and verifies the shape of the control points.
368-384: Custom normal vector test looks good.The test correctly checks if the result is not None when a custom normal vector is provided.
386-404: Invalid inputs test looks good.The test correctly uses
pytest.raisesto check for expected exceptions when invalid inputs are provided.
406-421: Rational splines test looks good.The test correctly checks if the result is not None and verifies that the result is rational.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- splinepy/bspline.py (1 hunks)
Additional comments not posted (1)
splinepy/bspline.py (1)
Line range hint
202-220: Confirm removal of logging statements fromremove_knotsmethod.The removal of logging statements might impact debugging and monitoring of knot removal operations. Consider maintaining a minimal level of logging or providing an alternative method for monitoring these operations.
Verification successful
Logging statements have been successfully removed from the
remove_knotsmethod.
- No logging statements were found in the
remove_knotsmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of logging statements from the remove_knots method. # Test: Search for logging statements in the remove_knots method. Expect: No logging statements. rg --type python --pcre2 'def remove_knots\(.+\)(?:\n.*){0,20}?self\._logd\('Length of output: 110
Script:
#!/bin/bash # Description: Verify the removal of logging statements from the remove_knots method. # Test: Search for logging statements in the remove_knots method. Expect: No logging statements. rg 'def remove_knots\(.+\)(?:\n.*){0,20}?self\._logd\('Length of output: 249
Script:
#!/bin/bash # Description: Verify the removal of logging statements from the remove_knots method. # Test: Search for logging statements in the remove_knots method. Expect: No logging statements. rg --multiline 'def remove_knots\(.+\)(?:\n.*){0,20}?self\._logd\('Length of output: 69
tests/helpme/test_create.py
Outdated
| control_points=[[0, 0], [0.5, 1], [1, 0]], | ||
| knot_vectors=[[0, 0, 0, 1, 1, 1]], | ||
| ) | ||
| invalid_trajectory = "invalid_trajectory" |
There was a problem hiding this comment.
looking at this, I think error message should be more informative
There was a problem hiding this comment.
This is no error message (you probably know that).
Otherwise, I don't know exactly what you mean. Should I raise an error message with the pytest.raises?
There was a problem hiding this comment.
Actionable comments posted: 13
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (2)
examples/show_swept.py (1)
10-10: Clean up the example.Remove commented-out code to improve readability.
- ### TRAJECTORY ###Likely invalid or redundant comment.
splinepy/helpme/create.py (1)
301-337: Ensure consistency in parameter naming and documentation.Ensure that the parameter
cross_sectionin the documentation matches the function signature.- cross_section : Spline + cross_section : SplineLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 11
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (5)
examples/show_swept.py (3)
143-143: Remove unnecessary sys.exit() call.The
sys.exit()call is unnecessary and can be removed to allow further code execution if needed.- sys.exit()Likely invalid or redundant comment.
1-6: Organize imports for better readability.Group standard library imports together, followed by third-party imports, and then local imports.
import sys import numpy as np import gustaf as gus import splinepyLikely invalid or redundant comment.
186-188: Clarify export file name.The file name
testmeshmesh.meshappears to be a typo. Consider renaming it to a more meaningful name.- splinepy.io.mfem.export("testmeshmesh.mesh", projection) + splinepy.io.mfem.export("swept_surface.mesh", projection)Likely invalid or redundant comment.
splinepy/helpme/create.py (2)
354-358: Check trajectory para_dim before type check.Check the
trajectory.para_dimbefore type checking for better error handling.- if not trajectory.para_dim == 1: - raise NotImplementedError("Trajectory must be 1D") - if not len(cross_section_normal) == 3: - raise ValueError("Cross section normal must be 3D") + if trajectory.para_dim != 1: + raise ValueError("Trajectory must have a parametric dimension of 1") + if cross_section_normal is not None and len(cross_section_normal) != 3: + raise ValueError("Cross section normal must be a 3D vector")
301-343: Correct the typo in the documentation.The parameter
crossectionin the documentation should be corrected tocross_sectionto match the function signature.- cross_section : Spline + cross_section : SplineLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 12
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (2)
examples/show_swept.py (2)
186-188: Clarify export file name.The file name
testmeshmesh.meshappears to be a typo. Consider renaming it to a more meaningful name.- splinepy.io.mfem.export("testmeshmesh.mesh", projection) + splinepy.io.mfem.export("swept_surface.mesh", projection)Likely invalid or redundant comment.
27-88: Remove commented-out code or move it to documentation.The commented-out code for different trajectory examples can be removed or moved to a separate documentation or example file.
- # closed 3D questionmarkLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (5)
splinepy/helpme/create.py (5)
359-362: Improve error messages for input type checks.The error messages could be more informative about what types are expected.
- raise NotImplementedError("Sweep only works for splines") + raise TypeError("cross_section must be an instance of Spline")- raise NotImplementedError("Sweep only works for splines") + raise TypeError("trajectory must be an instance of Spline")
373-377: Improve debug message for setting default value.The debug message for setting the default value of
cross_section_normalcould be more informative.- _log.debug("No cross section normal given. Defaulting to [0, 0, 1].") + _log.debug("No cross_section_normal given. Defaulting to [0, 0, 1].")
387-472: Refactor thetransformation_matricesfunction for better readability and maintainability.The nested function is lengthy and complex. Consider breaking it down into smaller, more manageable sub-functions.
def calculate_tangent_vector(traj, par_value): x = traj.derivative([par_value[0]], [1]) return (x / _np.linalg.norm(x)).ravel() def calculate_normal_vector(x): vec = [-x[1], x[0], -x[2]] if _np.linalg.norm(_np.cross(x, vec)) > _settings.TOLERANCE: return _np.cross(x, vec) / _np.linalg.norm(_np.cross(x, vec)) else: vec = [x[2], -x[1], x[0]] return _np.cross(x, vec) / _np.linalg.norm(_np.cross(x, vec)) def calculate_rotation_matrix(cross_section_normal): angle_of_cs_normal = _np.arctan2(cross_section_normal[2], cross_section_normal[0]) R = _np.array([ [_np.cos(angle_of_cs_normal), 0, _np.sin(angle_of_cs_normal)], [0, 1, 0], [_np.sin(angle_of_cs_normal), 0, _np.cos(angle_of_cs_normal)], ]) return _np.where(_np.abs(R) < _settings.TOLERANCE, 0, R) def transformation_matrices(traj, par_value, cross_section_normal): x = calculate_tangent_vector(traj, par_value) B = [calculate_normal_vector(x)] T, A = [], [] for i in range(len(par_value)): x = calculate_tangent_vector(traj, [par_value[i]]) B.append(B[i] - _np.dot(B[i], x) * x) B[i + 1] = B[i + 1] / _np.linalg.norm(B[i + 1]) z = B[i + 1] y = _np.cross(z, x) T.append(_np.vstack((x, y, z))) A.append(_np.linalg.inv(T[i])) R = calculate_rotation_matrix(cross_section_normal) return A, R
483-503: Optimize the rotation matrix computation.The rotation matrix computation can be optimized by using numpy functions directly.
- angle_of_cs_normal = _np.arctan2(cross_section_normal[2], cross_section_normal[0]) - R = _arr.rotation_matrix_around_axis( - axis=[0, 1, 0], rotation=angle_of_cs_normal, degree=False - ) + R = _np.eye(3) + angle_of_cs_normal = _np.arctan2(cross_section_normal[2], cross_section_normal[0]) + R[0, 0], R[0, 2] = _np.cos(angle_of_cs_normal), _np.sin(angle_of_cs_normal) + R[2, 0], R[2, 2] = -_np.sin(angle_of_cs_normal), _np.cos(angle_of_cs_normal) - R = _np.where(_np.abs(R) < _settings.TOLERANCE, 0, R)
505-573: Refactor the sweeping process for better readability and maintainability.The sweeping process is lengthy and complex. Consider breaking it down into smaller, more manageable sub-functions.
def translate_to_origin(cross_section): cross_para_center = _np.mean(cross_section.parametric_bounds, axis=0) cs_center = cross_section.evaluate(cross_para_center.reshape(-1, cross_section.para_dim)).ravel() cross_section.control_points -= cs_center return cross_section def set_cross_section_control_points(trajectory, cross_section, par_value, A, R, set_on_trajectory): swept_spline_cps = [] for i, par_val in enumerate(par_value): temp_csp = [] if set_on_trajectory: evals = trajectory.evaluate([par_val]).ravel() for cscp in cross_section.control_points: normal_cscp = _np.matmul(R, cscp) normal_cscp = _np.matmul(A[i], normal_cscp) if set_on_trajectory: normal_cscp += evals else: normal_cscp += trajectory.control_points[i] temp_csp.append(normal_cscp) swept_spline_cps.append(_np.array(temp_csp)) return swept_spline_cps cross_section = translate_to_origin(cross_section) A, R = transformation_matrices(trajectory, par_value, cross_section_normal) swept_spline_cps = set_cross_section_control_points(trajectory, cross_section, par_value, A, R, set_on_trajectory)
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- examples/show_swept.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (5)
examples/show_swept.py (5)
10-32: Trajectory definition is clear and well-implemented.The trajectory for the swept surface is defined clearly using a dictionary that specifies the degrees, knot vectors, and control points, which is then used to create a BSpline object. This is a standard and effective way to define splines in the
splinepylibrary.
39-46: Cross-section creation is effectively handled.The script effectively creates various types of cross-sections using helper functions from
splinepy.helpme.create, which simplifies the creation process. Converting these to NURBS is appropriate for ensuring compatibility with operations that may involve rational splines.
55-101: Sweeping process is versatile and well-demonstrated.The script showcases the versatility of the
sweptfunction by creating multiple swept surfaces and solids with various configurations. Each swept object is created with clear and appropriate parameters, effectively demonstrating the capabilities of the sweeping functionality.
113-138: Visualization is effectively implemented.The script uses the
gustaflibrary to visualize the trajectory, cross-sections, and swept surfaces. Different visualization configurations are applied to different objects, which helps in clearly distinguishing between them and effectively demonstrates the results of the sweeping process.
1-5: Organize imports for better readability.Group standard library imports together, followed by third-party imports, and then local imports. This improves readability and maintainability.
import gustaf as gus import numpy as np import splinepyLikely invalid or redundant comment.
Comments failed to post (1)
splinepy/helpme/create.py (1)
301-578: Comprehensive review of the
sweptfunction.
Correctness and Logic:
- The function handles the creation of a swept spline by manipulating control points along a trajectory. The logic follows expected operations for such functionality, including transformations and rotations.
Performance:
- The function performs multiple operations on arrays and matrices. Consider using numpy built-in functions for potential performance improvements.
Readability and Maintainability:
- The function is complex and lengthy, which could affect maintainability. Consider breaking it down into smaller, more manageable sub-functions.
Error Handling:
- Proper checks are in place for the input types. More detailed error messages could be helpful, particularly when inputs do not meet expected conditions.
Documentation:
- The documentation is thorough, but ensure that the parameter
cross_sectionin the documentation is corrected to match the function signature.Security/PII Leakage:
- No personal identifiable information or security-sensitive operations appear to be involved.
Unit Tests:
- Unit tests are noted as missing in the PR checklist. It's crucial to add comprehensive tests, especially given the mathematical complexity of the operations involved.
Best Practices:
- The use of more numpy built-in functions for array manipulations could simplify the code and potentially improve performance.
Action Items:
- Correct the typo in the documentation.
- Consider refactoring to improve readability and maintainability.
- Add unit tests to ensure the function's correctness across various scenarios.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (4)
- examples/show_swept.py (1 hunks)
- splinepy/bspline.py (1 hunks)
- splinepy/helpme/create.py (1 hunks)
- tests/helpme/test_create.py (2 hunks)
Additional comments not posted (8)
examples/show_swept.py (5)
1-5: Imports are appropriate and well-organized.The imports are essential for the functionality demonstrated in the script and are correctly structured.
10-33: Trajectory definition is clear and effectively utilizes thesplinepylibrary.The trajectory is defined in a structured manner using a dictionary, which is then used to create a
BSplineobject. This method is clear and allows for easy adjustments to the trajectory parameters.
39-51: Cross-section definitions are well-implemented and demonstrate the flexibility of thesweptfunction.The script effectively uses helper functions from
splinepyto create various types of cross-sections, showcasing the versatility of thesweptfunction. This approach not only simplifies the creation process but also ensures consistency across different types of cross-sections.
55-92: Sweeping operations are correctly implemented and effectively demonstrate the capabilities of thesweptfunction.The script performs multiple sweeping operations using different combinations of trajectories and cross-sections. The use of parameters like
cross_section_normalandset_on_trajectoryeffectively demonstrates the customization options available in thesweptfunction.
113-138: Visualization section is well-implemented and provides clear visual outputs of the sweeping results.The script uses the
gustaflibrary to visualize the results of the sweeping operations. Different visualization calls are made for different types of results, effectively demonstrating the outcomes of the sweeping operations.splinepy/helpme/create.py (1)
301-578: Comprehensive Review of thesweptFunctionThe
sweptfunction introduces significant functionality to thesplinepylibrary, enabling the creation of complex geometries by sweeping a cross-section along a trajectory. Here's a detailed review based on various aspects:
Correctness and Logic:
- The function handles the creation of a swept spline by manipulating control points along a trajectory. The logic generally follows the expected operations for such functionality, including transformations and rotations.
- The use of transformation matrices and knot insertion based on curvature is appropriate for creating smooth transitions along the trajectory.
Performance:
- The function performs multiple operations on arrays and matrices, including inversions and multiplications. Consider optimizing these operations if profiling indicates a bottleneck, especially for large splines.
Readability and Maintainability:
- The function is lengthy and complex, which could affect maintainability. Consider breaking it down into smaller, more manageable sub-functions, especially the parts handling matrix transformations and knot insertion.
Error Handling:
- Proper checks are in place for the input types. However, more detailed error messages could be helpful, particularly when inputs do not meet expected conditions.
Documentation:
- The documentation is thorough, providing clear descriptions of parameters and the return type. Ensure that the parameter
cross_sectionin the documentation is corrected tocross_sectionto match the function signature.Security/PII Leakage:
- No personal identifiable information or security-sensitive operations appear to be involved.
Unit Tests:
- Unit tests are noted as missing in the PR checklist. It's crucial to add comprehensive tests, especially given the mathematical complexity of the operations involved.
Best Practices:
- The function could benefit from the use of more numpy built-in functions for array manipulations to simplify the code and potentially improve performance.
Action Items:
- Correct the typo in the documentation.
- Consider refactoring to improve readability and maintainability.
- Add unit tests to ensure the function's correctness across various scenarios.
splinepy/bspline.py (1)
Line range hint
1-1: Approved: Minor stylistic improvement.The removal of the blank line before the return statement in the
remove_knotsmethod improves code cleanliness without affecting functionality.tests/helpme/test_create.py (1)
402-423: Approved: Effective comparison between swept and extruded surfaces.The test function
test_swept_to_extrudedeffectively compares the control points of linear swept and extruded surfaces, ensuring consistency between thesweptandextrudedmethods under similar conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- tests/helpme/test_create.py (2 hunks)
Additional comments not posted (3)
tests/helpme/test_create.py (3)
402-423: LGTM!The test function
test_swept_to_extrudedeffectively compares the control points of a linear swept surface to those of an extruded surface derived from the same cross-section. This serves to confirm that both methods produce equivalent results under specific conditions, thereby reinforcing the correctness of the implementation.
425-512: LGTM!The test function
test_swept_control_point_placingeffectively checks the placement of control points within the swept surface. It ensures that the normal vector of the cross-section is consistent with the tangent vector of the trajectory, which is vital for maintaining the accuracy of geometric transformations during the sweeping process.
514-538: LGTM!The test function
test_swept_cs_centeringeffectively checks that the center of the cross-section aligns with the trajectory, validating the spatial correctness of the swept surface. This test evaluates the coordinates of the swept spline against the trajectory at a designated parameter value, ensuring adherence to expected geometric properties.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- examples/show_swept.py (1 hunks)
Additional comments not posted (2)
examples/show_swept.py (2)
39-46: LGTM!The cross-section creation code is well-implemented and the comments clearly explain each cross-section.
116-123: LGTM!The visualization code segments are well-implemented and provide a clear demonstration of the swept surfaces and solids. The comments clearly explain the purpose of each visualization window.
Also applies to: 131-147, 150-157
|
Good progress! If you are done with comments, can you please resolve the conversations? Feel free to leave a note if needed |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- examples/show_swept.py (1 hunks)
Additional comments not posted (7)
examples/show_swept.py (7)
1-4: LGTM!The imports are correctly used and necessary for the script's functionality.
39-46: LGTM!The cross-section creation is correctly implemented using the appropriate helper functions from the
splinepylibrary. The variable names are descriptive and convey the shape of the cross-sections.
48-51: LGTM!The custom normal vector is correctly defined, and the comment provides a clear explanation of when to use a custom normal vector.
115-123: LGTM!The visualization of the swept surface is correctly implemented using the
gustaflibrary. The variable names are descriptive and convey the objects being visualized.
149-157: LGTM!The visualization of the swept solids with rectangular cross-sections is correctly implemented using the
gustaflibrary. The variable names are descriptive and convey the objects being visualized.
6-35: Improve variable naming for clarity.The trajectory definition is clear and well-structured. However, consider renaming the variable
dict_trajectoryto a more descriptive name, such ashook_trajectory_dict, to clearly convey the shape of the trajectory.-dict_trajectory = { +hook_trajectory_dict = {Likely invalid or redundant comment.
126-147: Use consistent naming for visualization options.The visualization options are correctly adjusted using the
show_optionsdictionary. However, consider using thevis_optionskey instead ofshow_optionsfor consistency with the rest of the codebase.-swept_solid_disk_2.show_options["alpha"] = 0.3 -swept_solid_disk_3.show_options["alpha"] = 0.3 -trajectory.show_options["control_points"] = False +swept_solid_disk_2.vis_options["alpha"] = 0.3 +swept_solid_disk_3.vis_options["alpha"] = 0.3 +trajectory.vis_options["show_control_points"] = FalseLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (7)
splinepy/helpme/create.py (7)
360-381: Input validation looks good!The input validation section thoroughly checks the types of the input parameters and raises appropriate exceptions. It also sets a default value for
cross_section_normalif not provided.
384-390: Initialization is correct.The initialization section embeds the
trajectoryandcross_sectioninto 3D space and initializes thepar_valueusing Greville abscissae, which is the correct approach.
392-508: Transformation matrices calculation is comprehensive.The transformation matrices calculation section handles the computation of tangent and normal vectors, closed trajectories, and the rotation matrix for the cross-section normal vector. It also applies an additional rotation if
rotation_adaptionis provided.The logic is correct and follows the mathematical formulas from the NURBS book. The use of
_settings.TOLERANCEfor numerical comparisons is a good practice.
510-545: Sweeping process is well-structured.The sweeping process section translates the cross-section to the origin and iterates over the trajectory, applying the calculated transformations to the cross-section control points. The transformed control points are collected for spline creation.
The logic is correct and follows the expected steps for sweeping a cross-section along a trajectory.
546-578: Spline creation handles rational splines correctly.The spline creation section constructs a spline dictionary with the necessary components (degrees, knot vectors, control points) and handles rational splines by adding weights. It instantiates the appropriate spline type (NURBS or BSpline) based on the rationality of the input splines.
The logic is correct and ensures that the resulting spline maintains the properties of the input splines.
301-578: Docstring is comprehensive and includes an example.The docstring for the
sweptfunction is well-written and provides a clear explanation of the function's purpose, parameters, and return value. It also includes a helpful example demonstrating how to use the function.The inclusion of the mathematical reference from the NURBS book is a nice touch for users who want to dive deeper into the underlying theory.
301-301: Reminder: Address the missing unit tests.The PR checklist indicates that unit tests have been added, but it's important to ensure that the
sweptfunction is thoroughly tested, given the complexity of the operations involved.To verify the existence and comprehensiveness of the unit tests, you can run the following script:
If the tests are missing or inadequate, consider adding more test cases to cover different scenarios and edge cases.
Verification successful
Unit tests for the
sweptfunction are comprehensive and well-structured.The existing unit tests in
tests/helpme/test_create.pycover various aspects of thesweptfunction, including:
- Basic functionality
- Comparison with related functions (e.g., extruded surfaces)
- Geometric properties (control point placement and cross-section centering)
- Different input types (NURBS and BSpline)
- Randomized inputs for diverse scenarios
These tests appear to address the complexity of the
sweptfunction and cover potential edge cases. The PR checklist's indication that unit tests have been added is accurate and satisfactory.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and comprehensiveness of unit tests for the `swept` function. # Test: Check if a test file for the `swept` function exists. # Expect: At least one test file matching the pattern `test_swept*.py`. fd -e py -p "test_swept.*\.py$" # Test: Check if the test file contains test cases for various scenarios. # Expect: Test cases covering different input types, edge cases, and expected outputs. rg -i -C 5 "def test_swept_" $(fd -e py -p "test_swept.*\.py$")Length of output: 2710
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (4)
splinepy/helpme/create.py (4)
359-381: Comprehensive input validation and default value handling.The function performs thorough checks on the input types and dimensions, raising appropriate exceptions for invalid inputs. It also handles the default value for
cross_section_normalcorrectly.
383-392: Initialization steps are correct.The function correctly embeds the
trajectoryandcross_sectioninto 3D space and initializes thepar_valueusing Greville abscissae.
511-546: Sweeping process is implemented correctly.The function correctly translates the cross-section to the origin and iterates over the trajectory points, applying the necessary transformations to the cross-section control points. It also handles the positioning of the cross-section based on the
set_on_trajectoryflag.
547-579: Consider adding more detailed error handling for spline creation.The spline creation logic is correct, but adding more detailed error handling can improve robustness.
- dict_swept_spline = { - "degrees": [*cross_section.degrees, *trajectory.degrees], - "knot_vectors": [ - *cross_section.knot_vectors, - *trajectory.knot_vectors, - ], - "control_points": _np.asarray(swept_spline_cps).reshape(-1, cross_section.dim), - } + try: + dict_swept_spline = { + "degrees": [*cross_section.degrees, *trajectory.degrees], + "knot_vectors": [ + *cross_section.knot_vectors, + *trajectory.knot_vectors, + ], + "control_points": _np.asarray(swept_spline_cps).reshape(-1, cross_section.dim), + } + except Exception as e: + raise ValueError(f"Error creating swept spline dictionary: {e}")Likely invalid or redundant comment.
splinepy/helpme/create.py
Outdated
| "Division by zero occurred. Using alternative vector for B." | ||
| ) | ||
|
|
||
| # initialize transformation matrices and tangent-vector-collection | ||
| T = [] | ||
| A = [] | ||
| tang_collection = [] | ||
|
|
||
| # evaluating transformation matrices for each trajectory point | ||
| for i in range(len(par_value)): | ||
| # calculation according to NURBS Book, eq. 10.27 | ||
| # tangent vector e1 on trajectory at parameter value i | ||
| e1 = trajectory.derivative([par_value[i]], [1]) | ||
| e1 = (e1 / _np.linalg.norm(e1)).ravel() | ||
| # collecting tangent vectors for later use | ||
| tang_collection.append(e1) | ||
|
|
||
| # projecting B_(i) onto the plane normal to e1 | ||
| B.append(B[i] - _np.dot(B[i], e1) * e1) | ||
| B[i + 1] = B[i + 1] / _np.linalg.norm(B[i + 1]) | ||
|
|
||
| # defining e2 and e3 vectors | ||
| e3 = B[i + 1] | ||
| e2 = _np.cross(e3, e1) | ||
|
|
||
| # array of transformation matrices from global to local coordinates | ||
| T.append(_np.vstack((e1, e2, e3))) | ||
|
|
||
| # array of transformation matrices from local to global coordinates | ||
| A.append(_np.linalg.inv(T[i])) | ||
|
|
||
| # separate procedure, if trajectory is closed and B[0] != B[-1] | ||
| # recalculate B-vector and middle the values between B and B_rec | ||
| # according to NURBS Book, Piegl & Tiller, 2nd edition, p. 483 | ||
| is_trajectory_closed = _np.array_equal( | ||
| trajectory.evaluate([[0]]), trajectory.evaluate([par_value[-1]]) | ||
| ) | ||
| is_B_start_equal_B_end = _np.array_equal(B[0], B[-1]) | ||
|
|
||
| if is_trajectory_closed and not is_B_start_equal_B_end: | ||
| # reset transformation matrices | ||
| T = [] | ||
| A = [] | ||
| # preallocate B_rec | ||
| B_rec = [None] * len(B) | ||
| # make sure start of B_rec is equal to the end of B | ||
| B_rec[0] = B[-1] | ||
| # redo the calculation of B using tang_collection from before | ||
| # in order to avoid recalculating the tangent vectors; | ||
| # calculation according to NURBS Book, eq. 10.27 | ||
| for i in range(len(par_value)): | ||
| B_rec[i + 1] = ( | ||
| B_rec[i] | ||
| - _np.dot(B_rec[i], tang_collection[i]) * tang_collection[i] | ||
| ) | ||
| B_rec[i + 1] = B_rec[i + 1] / _np.linalg.norm(B_rec[i + 1]) | ||
| # middle point between B and B_rec | ||
| B_rec[i + 1] = (B[i + 1] + B_rec[i + 1]) * 0.5 | ||
| # normalizing B_rec | ||
| B_rec[i + 1] = B_rec[i + 1] / _np.linalg.norm(B_rec[i + 1]) | ||
| # defining e2 and e3 axis-vectors | ||
| e3 = B_rec[i + 1] | ||
| e2 = _np.cross(e3, tang_collection[i]) | ||
|
|
||
| # array of transformation matrices from global to local coordinates | ||
| T.append(_np.vstack((tang_collection[i], e2, e3))) | ||
|
|
||
| # array of transformation matrices from local to global coordinates | ||
| A.append(_np.linalg.inv(T[i])) | ||
|
|
||
| # check if the beginning and the end of the B-vector are the same | ||
| if not _np.allclose(B_rec[0], B_rec[-1], rtol=1e-3): | ||
| _log.warning( | ||
| "Vector calculation is not exact due to the " | ||
| "trajectory being closed in an uncommon way." | ||
| ) | ||
|
|
||
| ### ROTATION MATRIX ### | ||
|
|
||
| # calculate angle of cross section normal vector around e2-axis | ||
| angle_of_cs_normal = _np.arctan2( | ||
| cross_section_normal[2], cross_section_normal[0] | ||
| ) | ||
|
|
||
| # calculate rotation matrix for cross section normal vector | ||
| R = _arr.rotation_matrix_around_axis( | ||
| axis=[0, 1, 0], rotation=angle_of_cs_normal, degree=False | ||
| ) | ||
|
|
||
| # rotate cross section around trajectory tangent vector if wanted | ||
| if rotation_adaption is not None: | ||
| R = _np.matmul( | ||
| _arr.rotation_matrix_around_axis( | ||
| axis=[1, 0, 0], rotation=rotation_adaption, degree=False | ||
| ), | ||
| R, | ||
| ) | ||
|
|
||
| # remove numerical noise | ||
| R = _np.where(_np.abs(R) < _settings.TOLERANCE, 0, R) |
There was a problem hiding this comment.
Consider refactoring the transformation matrices calculation.
The transformation matrices calculation is lengthy and complex. Consider breaking it down into smaller, more manageable sub-functions for better readability and maintainability.
def calculate_tangent_vector(traj, par_value):
x = traj.derivative([par_value[0]], [1])
return (x / _np.linalg.norm(x)).ravel()
def calculate_normal_vector(x):
vec = [-x[1], x[0], -x[2]]
if _np.linalg.norm(_np.cross(x, vec)) > _settings.TOLERANCE:
return _np.cross(x, vec) / _np.linalg.norm(_np.cross(x, vec))
else:
vec = [x[2], -x[1], x[0]]
return _np.cross(x, vec) / _np.linalg.norm(_np.cross(x, vec))
def calculate_rotation_matrix(cross_section_normal, rotation_adaption):
angle_of_cs_normal = _np.arctan2(cross_section_normal[2], cross_section_normal[0])
R = _arr.rotation_matrix_around_axis(axis=[0, 1, 0], rotation=angle_of_cs_normal, degree=False)
if rotation_adaption is not None:
R = _np.matmul(_arr.rotation_matrix_around_axis(axis=[1, 0, 0], rotation=rotation_adaption, degree=False), R)
return _np.where(_np.abs(R) < _settings.TOLERANCE, 0, R)
def transformation_matrices(traj, par_value, cross_section_normal, rotation_adaption):
x = calculate_tangent_vector(traj, par_value)
B = [calculate_normal_vector(x)]
T, A = [], []
for i in range(len(par_value)):
x = calculate_tangent_vector(traj, [par_value[i]])
B.append(B[i] - _np.dot(B[i], x) * x)
B[i + 1] = B[i + 1] / _np.linalg.norm(B[i + 1])
z = B[i + 1]
y = _np.cross(z, x)
T.append(_np.vstack((x, y, z)))
A.append(_np.linalg.inv(T[i]))
R = calculate_rotation_matrix(cross_section_normal, rotation_adaption)
return A, RThere was a problem hiding this comment.
@j042 what do you think about it?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@j042 Can/should I add an entry about swept to the Readme? |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- splinepy/helpme/create.py (1 hunks)
Additional comments not posted (3)
splinepy/helpme/create.py (3)
359-380: Input validation looks good!The input validation section thoroughly checks the types and values of the input parameters, ensuring that they meet the expected criteria. The error messages are informative and help identify issues with the input.
382-391: Initialization and setup look good!The initialization and setup section correctly prepares the input splines and initializes the necessary parameters for the sweeping process. The use of
create.embeddedensures that both splines are in 3D space, which is required for the subsequent calculations.
561-593: Spline creation is handled correctly!The spline creation section correctly assembles the necessary components (degrees, knot vectors, control points, and weights) to create the swept spline. The handling of rational splines ensures that the weights are properly incorporated into the swept spline. The determination of the spline type based on the rationality of the input splines is a good approach.
| ### TRANSFORMATION MATRICES ### | ||
|
|
||
| # tangent vector 'e1' of trajectory at parameter value 0 | ||
| e1 = trajectory.derivative([par_value[0]], [1]) | ||
| e1 = (e1 / _np.linalg.norm(e1)).ravel() | ||
|
|
||
| # evaluating a vector normal to e1 | ||
| vec = [-e1[1], e1[0], -e1[2]] | ||
| B = [] | ||
| # avoid dividing by zero | ||
| if _np.linalg.norm(_np.cross(e1, vec)) > _settings.TOLERANCE: | ||
| B.append(_np.cross(e1, vec) / _np.linalg.norm(_np.cross(e1, vec))) | ||
| else: | ||
| vec = [e1[2], -e1[1], e1[0]] | ||
| B.append(_np.cross(e1, vec) / _np.linalg.norm(_np.cross(e1, vec))) | ||
| # add debug message | ||
| _log.debug( | ||
| "Division by zero occurred. Using alternative vector for B." | ||
| ) | ||
|
|
||
| # initialize transformation matrices and tangent-vector-collection | ||
| T = [] | ||
| A = [] | ||
| tang_collection = [] | ||
|
|
||
| # evaluating transformation matrices for each trajectory point | ||
| for i in range(len(par_value)): | ||
| # calculation according to NURBS Book, eq. 10.27 | ||
| # tangent vector e1 on trajectory at parameter value i | ||
| e1 = trajectory.derivative([par_value[i]], [1]) | ||
| e1 = (e1 / _np.linalg.norm(e1)).ravel() | ||
| # collecting tangent vectors for later use | ||
| tang_collection.append(e1) | ||
|
|
||
| # projecting B_(i) onto the plane normal to e1 | ||
| B.append(B[i] - _np.dot(B[i], e1) * e1) | ||
| B[i + 1] = B[i + 1] / _np.linalg.norm(B[i + 1]) | ||
|
|
||
| # defining e2 and e3 vectors | ||
| e3 = B[i + 1] | ||
| e2 = _np.cross(e3, e1) | ||
|
|
||
| # array of transformation matrices from global to local coordinates | ||
| T.append(_np.vstack((e1, e2, e3))) | ||
|
|
||
| # array of transformation matrices from local to global coordinates | ||
| A.append(_np.linalg.inv(T[i])) | ||
|
|
||
| # separate procedure, if trajectory is closed and B[0] != B[-1] | ||
| # recalculate B-vector and middle the values between B and B_rec | ||
| # according to NURBS Book, Piegl & Tiller, 2nd edition, p. 483 | ||
| is_trajectory_closed = _np.array_equal( | ||
| trajectory.evaluate([[0]]), trajectory.evaluate([par_value[-1]]) | ||
| ) | ||
| is_B_start_equal_B_end = _np.array_equal(B[0], B[-1]) | ||
|
|
||
| if is_trajectory_closed and not is_B_start_equal_B_end: | ||
| # reset transformation matrices | ||
| T = [] | ||
| A = [] | ||
| # preallocate B_rec | ||
| B_rec = [None] * len(B) | ||
| # make sure start of B_rec is equal to the end of B | ||
| B_rec[0] = B[-1] | ||
| # redo the calculation of B using tang_collection from before | ||
| # in order to avoid recalculating the tangent vectors; | ||
| # calculation according to NURBS Book, eq. 10.27 | ||
| for i in range(len(par_value)): | ||
| B_rec[i + 1] = ( | ||
| B_rec[i] | ||
| - _np.dot(B_rec[i], tang_collection[i]) * tang_collection[i] | ||
| ) | ||
| B_rec[i + 1] = B_rec[i + 1] / _np.linalg.norm(B_rec[i + 1]) | ||
| # middle point between B and B_rec | ||
| B_rec[i + 1] = (B[i + 1] + B_rec[i + 1]) * 0.5 | ||
| # normalizing B_rec | ||
| B_rec[i + 1] = B_rec[i + 1] / _np.linalg.norm(B_rec[i + 1]) | ||
| # defining e2 and e3 axis-vectors | ||
| e3 = B_rec[i + 1] | ||
| e2 = _np.cross(e3, tang_collection[i]) | ||
|
|
||
| # array of transformation matrices from global to local coordinates | ||
| T.append(_np.vstack((tang_collection[i], e2, e3))) | ||
|
|
||
| # array of transformation matrices from local to global coordinates | ||
| A.append(_np.linalg.inv(T[i])) | ||
|
|
||
| # check if the beginning and the end of the B-vector are the same | ||
| if not _np.allclose(B_rec[0], B_rec[-1], rtol=1e-3): | ||
| _log.warning( | ||
| "Vector calculation is not exact due to the " | ||
| "trajectory being closed in an uncommon way." | ||
| ) | ||
|
|
||
| ### ROTATION MATRIX ### |
There was a problem hiding this comment.
Transformation matrices calculation is correct but could be refactored.
The transformation matrices calculation section correctly implements the equations and logic described in the NURBS book. The handling of closed trajectories is a nice addition to ensure continuity.
However, the section is lengthy and complex. Consider breaking it down into smaller, more manageable sub-functions for better readability and maintainability.
Here's a suggestion for refactoring the transformation matrices calculation:
def calculate_tangent_vector(traj, par_value):
x = traj.derivative([par_value[0]], [1])
return (x / _np.linalg.norm(x)).ravel()
def calculate_normal_vector(x):
vec = [-x[1], x[0], -x[2]]
if _np.linalg.norm(_np.cross(x, vec)) > _settings.TOLERANCE:
return _np.cross(x, vec) / _np.linalg.norm(_np.cross(x, vec))
else:
vec = [x[2], -x[1], x[0]]
return _np.cross(x, vec) / _np.linalg.norm(_np.cross(x, vec))
def transformation_matrices(traj, par_value, cross_section_normal):
x = calculate_tangent_vector(traj, par_value)
B = [calculate_normal_vector(x)]
T, A = [], []
for i in range(len(par_value)):
x = calculate_tangent_vector(traj, [par_value[i]])
B.append(B[i] - _np.dot(B[i], x) * x)
B[i + 1] = B[i + 1] / _np.linalg.norm(B[i + 1])
z = B[i + 1]
y = _np.cross(z, x)
T.append(_np.vstack((x, y, z)))
A.append(_np.linalg.inv(T[i]))
return A| ### SWEEPING PROCESS ### | ||
|
|
||
| # evaluate center of cross-section and translate to origin | ||
| cross_para_center = _np.mean(cross_section.parametric_bounds, axis=0) | ||
| cs_center = cross_section.evaluate( | ||
| cross_para_center.reshape(-1, cross_section.para_dim) | ||
| ).ravel() | ||
| cross_section.control_points -= cs_center | ||
|
|
||
| # set cross-section control points along trajectory | ||
| swept_spline_cps = [] | ||
| for i, par_val in enumerate(par_value): | ||
| temp_csp = [] | ||
| # evaluate trajectory if user wants to set cross-section on trajectory. | ||
| if set_on_trajectory: | ||
| evals = trajectory.evaluate([par_val]).ravel() | ||
| # place every control point of cross-section separately | ||
| for cscp in cross_section.control_points: | ||
| # rotate cross-section in trajectory direction | ||
| normal_cscp = _np.matmul(R, cscp) | ||
| # transform cross-section to global coordinates | ||
| normal_cscp = _np.matmul(A[i], normal_cscp) | ||
| # check if user wants to place cross-section at | ||
| # evaluation points or control points of trajectory | ||
| if set_on_trajectory: | ||
| # translate cross-section to trajectory evaluation point | ||
| normal_cscp += evals | ||
| else: | ||
| # translate cross-section to trajectory control point | ||
| normal_cscp += trajectory.control_points[i] | ||
| # append control point to list | ||
| temp_csp.append(normal_cscp) | ||
|
|
||
| # collect all control points | ||
| swept_spline_cps.append(_np.array(temp_csp)) | ||
|
|
There was a problem hiding this comment.
Sweeping process is implemented correctly!
The sweeping process section correctly positions and orients the cross-section along the trajectory. The translation of the cross-section to the origin ensures that the rotation and transformation are applied correctly. The use of the rotation matrix and transformation matrices ensures that the cross-section follows the trajectory smoothly.
To improve readability, you can extract the translation of the cross-section to the origin into a separate function:
def translate_to_origin(cross_section):
cross_para_center = _np.mean(cross_section.parametric_bounds, axis=0)
cs_center = cross_section.evaluate(cross_para_center.reshape(-1, cross_section.para_dim)).ravel()
cross_section.control_points -= cs_center
return cross_section
cross_section = translate_to_origin(cross_section)| # calculate angle of cross-section normal vector around e2-axis | ||
| angle_of_cs_normal_1 = _np.arctan2( | ||
| cross_section_normal[2], cross_section_normal[0] | ||
| ) | ||
|
|
||
| # calculate angle of cross-section normal vector around e3-axis | ||
| angle_of_cs_normal_2 = _np.arctan2( | ||
| cross_section_normal[1], cross_section_normal[2] | ||
| ) | ||
|
|
||
| # calculate rotation matrix for cross-section normal vector | ||
| R1 = _arr.rotation_matrix_around_axis( | ||
| axis=[0, 1, 0], rotation=angle_of_cs_normal_1, degree=False | ||
| ) | ||
| R2 = _arr.rotation_matrix_around_axis( | ||
| axis=[0, 0, 1], rotation=angle_of_cs_normal_2, degree=False | ||
| ) | ||
| R = _np.matmul(R2, R1) | ||
|
|
||
| # rotate cross-section around trajectory tangent vector (e1) if wanted | ||
| if rotation_adaption is not None: | ||
| R = _np.matmul( | ||
| _arr.rotation_matrix_around_axis( | ||
| axis=[1, 0, 0], rotation=rotation_adaption, degree=False | ||
| ), | ||
| R, | ||
| ) | ||
|
|
||
| # remove numerical noise | ||
| R = _np.where(_np.abs(R) < _settings.TOLERANCE, 0, R) | ||
|
|
||
| ### SWEEPING PROCESS ### | ||
|
|
||
| # evaluate center of cross-section and translate to origin | ||
| cross_para_center = _np.mean(cross_section.parametric_bounds, axis=0) | ||
| cs_center = cross_section.evaluate( | ||
| cross_para_center.reshape(-1, cross_section.para_dim) | ||
| ).ravel() | ||
| cross_section.control_points -= cs_center | ||
|
|
||
| # set cross-section control points along trajectory | ||
| swept_spline_cps = [] | ||
| for i, par_val in enumerate(par_value): | ||
| temp_csp = [] | ||
| # evaluate trajectory if user wants to set cross-section on trajectory. | ||
| if set_on_trajectory: | ||
| evals = trajectory.evaluate([par_val]).ravel() | ||
| # place every control point of cross-section separately | ||
| for cscp in cross_section.control_points: | ||
| # rotate cross-section in trajectory direction | ||
| normal_cscp = _np.matmul(R, cscp) | ||
| # transform cross-section to global coordinates | ||
| normal_cscp = _np.matmul(A[i], normal_cscp) | ||
| # check if user wants to place cross-section at | ||
| # evaluation points or control points of trajectory | ||
| if set_on_trajectory: | ||
| # translate cross-section to trajectory evaluation point | ||
| normal_cscp += evals | ||
| else: | ||
| # translate cross-section to trajectory control point | ||
| normal_cscp += trajectory.control_points[i] | ||
| # append control point to list | ||
| temp_csp.append(normal_cscp) | ||
|
|
||
| # collect all control points | ||
| swept_spline_cps.append(_np.array(temp_csp)) | ||
|
|
||
| # create spline dictionary | ||
| dict_swept_spline = { | ||
| "degrees": [*cross_section.degrees, *trajectory.degrees], | ||
| "knot_vectors": [ | ||
| *cross_section.knot_vectors, | ||
| *trajectory.knot_vectors, | ||
| ], | ||
| "control_points": _np.asarray(swept_spline_cps).reshape( | ||
| -1, cross_section.dim | ||
| ), | ||
| } | ||
|
|
||
| # add weights properly if spline is rational | ||
| if cross_section.is_rational or trajectory.is_rational: | ||
|
|
||
| def weights(spline): | ||
| if spline.is_rational: | ||
| return spline.weights | ||
| return _np.ones(spline.control_points.shape[0]) | ||
|
|
||
| trajectory_weights = weights(trajectory) | ||
| cross_section_weights = weights(cross_section) | ||
| dict_swept_spline["weights"] = _np.outer( | ||
| trajectory_weights, cross_section_weights | ||
| ).reshape(-1, 1) | ||
| spline_type = _NURBS | ||
| else: | ||
| spline_type = _BSpline | ||
|
|
||
| # create swept spline | ||
| swept_spline = spline_type(**dict_swept_spline) | ||
|
|
||
| return swept_spline |
There was a problem hiding this comment.
Comprehensive implementation of the sweeping process!
The swept function is a well-structured and comprehensive implementation of the sweeping process for creating spline surfaces and solids. It follows the mathematical principles described in the NURBS book and handles various cases, such as closed trajectories and rational splines, to ensure correct results.
The code is modular and can be further improved by refactoring complex sections into smaller, reusable functions. Overall, the swept function is a valuable addition to the splinepy library and provides a powerful tool for creating swept surfaces and solids.
To further enhance the code, consider refactoring the complex sections, such as the transformation matrices calculation and the sweeping process, into separate functions. This will improve readability, maintainability, and reusability of the code.
| ### ROTATION MATRIX ### | ||
| # rotates cross-section normal vector into global e1 direction | ||
|
|
||
| # skip calculation if cross-section normal vector is default | ||
| if _np.array_equal(cross_section_normal, _np.array([0, 0, 1])): | ||
| R = _np.array([[0.0, 0.0, 1.0], [0.0, 1.0, 0.0], [-1.0, 0.0, 0.0]]) | ||
| # else, calculate rotation matrix for given cross-section normal vector | ||
| else: | ||
| # calculate angle of cross-section normal vector around e2-axis | ||
| angle_of_cs_normal_1 = _np.arctan2( | ||
| cross_section_normal[2], cross_section_normal[0] | ||
| ) | ||
|
|
||
| # calculate angle of cross-section normal vector around e3-axis | ||
| angle_of_cs_normal_2 = _np.arctan2( | ||
| cross_section_normal[1], cross_section_normal[2] | ||
| ) | ||
|
|
||
| # calculate rotation matrix for cross-section normal vector | ||
| R1 = _arr.rotation_matrix_around_axis( | ||
| axis=[0, 1, 0], rotation=angle_of_cs_normal_1, degree=False | ||
| ) | ||
| R2 = _arr.rotation_matrix_around_axis( | ||
| axis=[0, 0, 1], rotation=angle_of_cs_normal_2, degree=False | ||
| ) | ||
| R = _np.matmul(R2, R1) | ||
|
|
||
| # rotate cross-section around trajectory tangent vector (e1) if wanted | ||
| if rotation_adaption is not None: | ||
| R = _np.matmul( | ||
| _arr.rotation_matrix_around_axis( | ||
| axis=[1, 0, 0], rotation=rotation_adaption, degree=False | ||
| ), | ||
| R, | ||
| ) | ||
|
|
||
| # remove numerical noise | ||
| R = _np.where(_np.abs(R) < _settings.TOLERANCE, 0, R) |
There was a problem hiding this comment.
Rotation matrix calculation is correct!
The rotation matrix calculation section correctly computes the rotation matrix for the cross-section normal vector. The handling of the default case simplifies the calculation, and the use of the rotation_matrix_around_axis function ensures that the rotation is performed correctly.
As a small optimization, you can simplify the default case by directly assigning the rotation matrix instead of using _np.array:
- R = _np.array([[0.0, 0.0, 1.0], [0.0, 1.0, 0.0], [-1.0, 0.0, 0.0]])
+ R = _np.eye(3)
+ R[0, 2], R[2, 0] = 1, -1Committable 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.
| ### ROTATION MATRIX ### | |
| # rotates cross-section normal vector into global e1 direction | |
| # skip calculation if cross-section normal vector is default | |
| if _np.array_equal(cross_section_normal, _np.array([0, 0, 1])): | |
| R = _np.array([[0.0, 0.0, 1.0], [0.0, 1.0, 0.0], [-1.0, 0.0, 0.0]]) | |
| # else, calculate rotation matrix for given cross-section normal vector | |
| else: | |
| # calculate angle of cross-section normal vector around e2-axis | |
| angle_of_cs_normal_1 = _np.arctan2( | |
| cross_section_normal[2], cross_section_normal[0] | |
| ) | |
| # calculate angle of cross-section normal vector around e3-axis | |
| angle_of_cs_normal_2 = _np.arctan2( | |
| cross_section_normal[1], cross_section_normal[2] | |
| ) | |
| # calculate rotation matrix for cross-section normal vector | |
| R1 = _arr.rotation_matrix_around_axis( | |
| axis=[0, 1, 0], rotation=angle_of_cs_normal_1, degree=False | |
| ) | |
| R2 = _arr.rotation_matrix_around_axis( | |
| axis=[0, 0, 1], rotation=angle_of_cs_normal_2, degree=False | |
| ) | |
| R = _np.matmul(R2, R1) | |
| # rotate cross-section around trajectory tangent vector (e1) if wanted | |
| if rotation_adaption is not None: | |
| R = _np.matmul( | |
| _arr.rotation_matrix_around_axis( | |
| axis=[1, 0, 0], rotation=rotation_adaption, degree=False | |
| ), | |
| R, | |
| ) | |
| # remove numerical noise | |
| R = _np.where(_np.abs(R) < _settings.TOLERANCE, 0, R) | |
| ### ROTATION MATRIX ### | |
| # rotates cross-section normal vector into global e1 direction | |
| # skip calculation if cross-section normal vector is default | |
| if _np.array_equal(cross_section_normal, _np.array([0, 0, 1])): | |
| R = _np.eye(3) | |
| R[0, 2], R[2, 0] = 1, -1 | |
| # else, calculate rotation matrix for given cross-section normal vector | |
| else: | |
| # calculate angle of cross-section normal vector around e2-axis | |
| angle_of_cs_normal_1 = _np.arctan2( | |
| cross_section_normal[2], cross_section_normal[0] | |
| ) | |
| # calculate angle of cross-section normal vector around e3-axis | |
| angle_of_cs_normal_2 = _np.arctan2( | |
| cross_section_normal[1], cross_section_normal[2] | |
| ) | |
| # calculate rotation matrix for cross-section normal vector | |
| R1 = _arr.rotation_matrix_around_axis( | |
| axis=[0, 1, 0], rotation=angle_of_cs_normal_1, degree=False | |
| ) | |
| R2 = _arr.rotation_matrix_around_axis( | |
| axis=[0, 0, 1], rotation=angle_of_cs_normal_2, degree=False | |
| ) | |
| R = _np.matmul(R2, R1) | |
| # rotate cross-section around trajectory tangent vector (e1) if wanted | |
| if rotation_adaption is not None: | |
| R = _np.matmul( | |
| _arr.rotation_matrix_around_axis( | |
| axis=[1, 0, 0], rotation=rotation_adaption, degree=False | |
| ), | |
| R, | |
| ) | |
| # remove numerical noise | |
| R = _np.where(_np.abs(R) < _settings.TOLERANCE, 0, R) |
Overview
sweptis a tool within thesplinepy.helpme.createenvironment to provide sweeping a cross section along a trajectory.Addressed issues
Showcase
In order to create a spline patch of a spaghetti pasta, we can pass a circular cross-section and an arbitrary trajectory. The result can be both a 3-dimensional volume spline or a 2-dimensional shell spline, depending on the dimension of your cross-section.
Checklists
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests