Swept Surface #438
Swept Surface #438
Conversation
|
Nice, can you post some screenshots? |
Sure! Be aware that work is still in progress.
|
|
Warning Rate limit exceeded@OberGue has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates introduce a new function Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SplinePy as splinepy.create.py
User->>SplinePy: Call swept(cross_section, trajectory)
SplinePy-->>User: Return swept_spline
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.
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.
splinepy/helpme/create.py
Outdated
| if not isinstance(cross_section, (_BSpline, _NURBS)): | ||
| raise TypeError( | ||
| "cross_section must be an instance of BSpline or NURBS" | ||
| ) | ||
| if not isinstance(trajectory, (_BSpline, _NURBS)): | ||
| raise TypeError("trajectory must be an instance of BSpline or NURBS") |
There was a problem hiding this comment.
Can you just check if they are spline and if they are, just ensure that they are BSpline or NURBS?
There was a problem hiding this comment.
Done - please check if it's in the way you like it.
splinepy/helpme/create.py
Outdated
| raise TypeError("trajectory must be of parametric dimension 1") | ||
| if not isinstance(set_on_trajectory, bool): | ||
| raise TypeError("set_on_trajectory must be a boolean") | ||
| if not isinstance(rotation_adaption, (float, int, type(None))): |
There was a problem hiding this comment.
numpy numbers also has its own type. Can you remove this line and cast input to float at l514?
| cross_section = cross_section.create.embedded(3) | ||
|
|
||
| # initialize parameter values | ||
| par_value = trajectory.greville_abscissae() |
There was a problem hiding this comment.
haven't we discussed that this should be input values?
There was a problem hiding this comment.
We talked about providing them as input values in order to create an additional option for the user. I tried implementing it, however, problems with the calculation of the swept-spline's weights occured. These weights are calculated via the weights of trajectory and cross-section and this calculation would have to be changed. Since I assume this additional feature of providing parameter values is not quite urgent, I thrusted this issue aside for the moment.
splinepy/helpme/create.py
Outdated
| 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))) |
There was a problem hiding this comment.
you can only compute cross once here instead
splinepy/helpme/create.py
Outdated
| 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))) |
There was a problem hiding this comment.
you can only compute cross once here instead
splinepy/helpme/create.py
Outdated
|
|
||
| # 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]) |
|
|
||
| # test the basic functionality of the swept-function | ||
| def test_swept_basic_functionality(): | ||
| cross_section = splinepy.NURBS( |
There was a problem hiding this comment.
isn't this one same as a fixture we already have?
There was a problem hiding this comment.
I can't find a similar fixture.
tests/helpme/test_create.py
Outdated
|
|
||
|
|
||
| # check if cross-section's center lays on the trajectory | ||
| def test_swept_cs_centering(np_rng): |
There was a problem hiding this comment.
| def test_swept_cs_centering(np_rng): | |
| def test_swept_cross_section_centering(np_rng): |
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 (3)
splinepy/helpme/create.py (3)
487-491: Verify the tolerance used when checking B vectors for closed trajectories
The tolerance rtol=1e-3 is used when checking if B_rec[0] is close to B_rec[-1]. Please verify whether this tolerance is appropriate for your application's precision requirements, as it may affect the accuracy of the sweeping process for closed trajectories.
493-530: 🧹 Nitpick (assertive)
Optimize the rotation matrix computation
The rotation matrix calculation can be simplified for efficiency and readability. Instead of calculating separate rotations (R1 and R2), consider computing the rotation matrix directly.
Apply this diff to simplify the rotation matrix computation:
# 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 angle of cross-section normal vector around e2-axis
- angle_of_cs_normal_1 = _np.arctan2(
+ angle_of_cs_normal = _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 matrices 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)
+ R = _np.eye(3)
+ 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)394-531: 🧹 Nitpick (assertive)
Consider refactoring complex sections into smaller functions
The code sections for calculating transformation matrices and handling special cases are lengthy and complex. Refactoring them into smaller, well-named helper functions can improve readability and maintainability.
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 (4)
splinepy/helpme/create.py (4)
455-500: LGTM: Closed trajectory handling is well implemented
The code correctly implements the algorithm from the NURBS book for handling closed trajectories, including proper warning messages for edge cases.
583-615: LGTM: Spline creation is well implemented
The code correctly creates the final spline with proper handling of rational splines and weights.
375-376: 🛠️ Refactor suggestion
Use ValueError for invalid trajectory parametric dimension
The check for trajectory.para_dim compares a value rather than a type, so it should raise a ValueError.
Apply this diff to fix the error type:
if not trajectory.para_dim == 1:
- raise TypeError("trajectory must be of parametric dimension 1")
+ raise ValueError("trajectory must have a parametric dimension of 1")Likely invalid or redundant comment.
301-354: 🛠️ Refactor suggestion
Fix spelling: 'rotation_adaption' should be 'rotation_adaptation'
The parameter name 'rotation_adaption' is misspelled throughout the code. The correct spelling is 'rotation_adaptation'.
Apply this diff to fix the spelling:
def swept(
cross_section,
trajectory,
cross_section_normal=None,
set_on_trajectory=False,
- rotation_adaption=None,
+ rotation_adaptation=None,
):Likely invalid or redundant comment.
| # initialize parameter values | ||
| par_value = trajectory.greville_abscissae() | ||
| par_value = par_value.reshape(-1, 1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Greville abscissae with input values
As discussed in past comments, parameter values should be provided as input rather than using Greville abscissae.
Consider adding a new parameter to accept input values:
def swept(
cross_section,
trajectory,
cross_section_normal=None,
set_on_trajectory=False,
rotation_adaption=None,
+ parameter_values=None,
):
...
- par_value = trajectory.greville_abscissae()
- par_value = par_value.reshape(-1, 1)
+ if parameter_values is None:
+ par_value = trajectory.greville_abscissae().reshape(-1, 1)
+ else:
+ par_value = _np.asarray(parameter_values).reshape(-1, 1)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 9
📜 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(2 hunks)
🔇 Additional comments (5)
splinepy/helpme/create.py (5)
375-376: 🛠️ Refactor suggestion
Use ValueError for invalid trajectory parametric dimension.
The check for trajectory.para_dim compares a value rather than a type, so it should raise a ValueError.
Apply this diff:
if not trajectory.para_dim == 1:
- raise TypeError("trajectory must be of parametric dimension 1")
+ raise ValueError("trajectory must have a parametric dimension of 1")Likely invalid or redundant comment.
554-554: 🧹 Nitpick (assertive)
Use inplace operation for control points translation.
The control points translation can be done inplace for better performance.
Apply this diff:
-cross_section.control_points -= cs_center
+cross_section.control_points -= cs_center[None, :] # Broadcasting for inplace operationLikely invalid or redundant comment.
529-542: 🧹 Nitpick (assertive)
Improve type checking for rotation_adaption.
The type checking and conversion for rotation_adaption can be simplified.
Apply this diff:
if rotation_adaption is not None:
- try:
- rotation_adaption = float(rotation_adaption)
- except TypeError:
- raise TypeError(
- "rotation_adaption must be a number (float, int) or None"
- )
+ if not isinstance(rotation_adaption, (int, float)):
+ raise TypeError("rotation_adaption must be a number (float, int) or None")
+ rotation_adaption = float(rotation_adaption)Likely invalid or redundant comment.
394-396: 🛠️ Refactor suggestion
Add support for custom parameter values.
As discussed in past comments, parameter values should be provided as input rather than using Greville abscissae.
Consider adding a new parameter to accept input values:
def swept(
cross_section,
trajectory,
cross_section_normal=None,
set_on_trajectory=False,
rotation_adaption=None,
+ parameter_values=None,
):
...
- par_value = trajectory.greville_abscissae()
- par_value = par_value.reshape(-1, 1)
+ if parameter_values is None:
+ par_value = trajectory.greville_abscissae().reshape(-1, 1)
+ else:
+ par_value = _np.asarray(parameter_values).reshape(-1, 1)Likely invalid or redundant comment.
301-354: 🧹 Nitpick (assertive)
Fix spelling of 'rotation_adaption' parameter.
The parameter name should be 'rotation_adaptation' instead of 'rotation_adaption' to follow correct English spelling.
Apply this diff:
def swept(
cross_section,
trajectory,
cross_section_normal=None,
set_on_trajectory=False,
- rotation_adaption=None,
+ rotation_adaptation=None,
):Likely 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 (2)
splinepy/helpme/create.py (2)
388-388: 🧹 Nitpick (assertive)
Use consistent logging methods for better maintainability
Instead of using _log.debug(), consider using the spline's _logd() method for logging debug messages to maintain consistency across the codebase.
Apply this diff:
- _log.debug("No cross_section_normal given. Defaulting to [0, 0, 1].")
+ trajectory._logd("No cross_section_normal given. Defaulting to [0, 0, 1].")Likely invalid or redundant comment.
575-578: Ensure correct indexing when accessing trajectory.control_points
When set_on_trajectory is False, the code assumes that the number of iterations i matches the length of trajectory.control_points. If par_value has more values than trajectory.control_points, this could result in an IndexError. Verify that par_value corresponds correctly to trajectory.control_points.
Run the following script to check the lengths:
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