Conversation
WalkthroughThe changes update multiple files in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TileBase
participant TileClass
participant ParameterProcessor
User->>TileClass: create_tile(parameters, parameter_sensitivities)
TileClass->>TileBase: _process_input(parameters, parameter_sensitivities)
TileBase-->>TileClass: processed_parameters, n_derivatives, derivatives
TileClass->>TileClass: construct_splines_with_derivatives()
TileClass-->>User: splines, derivatives
sequenceDiagram
participant User
participant TileClass
participant TileBase
User->>TileClass: create_tile(closure=direction)
TileClass->>TileBase: _process_input(parameters, parameter_sensitivities)
TileBase-->>TileClass: processed_parameters, n_derivatives, derivatives
TileClass->>TileClass: _closing_tile(closure=direction)
TileClass-->>User: closing_splines, derivatives
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (16)
- examples/show_microstructures.py (2 hunks)
- splinepy/microstructure/microstructure.py (2 hunks)
- splinepy/microstructure/tiles/init.py (2 hunks)
- splinepy/microstructure/tiles/chi.py (2 hunks)
- splinepy/microstructure/tiles/cross_2d.py (2 hunks)
- splinepy/microstructure/tiles/cross_3d.py (2 hunks)
- splinepy/microstructure/tiles/cross_3d_linear.py (1 hunks)
- splinepy/microstructure/tiles/cube_void.py (2 hunks)
- splinepy/microstructure/tiles/double_lattice.py (2 hunks)
- splinepy/microstructure/tiles/ellips_v_oid.py (1 hunks)
- splinepy/microstructure/tiles/hollow_cube.py (2 hunks)
- splinepy/microstructure/tiles/hollow_octagon.py (4 hunks)
- splinepy/microstructure/tiles/hollow_octagon_extrude.py (2 hunks)
- splinepy/microstructure/tiles/inverse_cross_3d.py (33 hunks)
- splinepy/microstructure/tiles/snappy.py (5 hunks)
- tests/test_microstructure.py (1 hunks)
Additional comments not posted (79)
splinepy/microstructure/tiles/__init__.py (2)
11-11: LGTM!The addition of
cross_3dto the import statements is correct and consistent with the other imports.
46-46: LGTM!The addition of
cross_3dto the exported module list is correct and consistent with the other exports.splinepy/microstructure/tiles/chi.py (7)
19-19: LGTM!The change in
_evaluation_pointsfrom_np.array([[0.5, 0.5]])to_np.array([[0.5]])is correct and consistent with the rest of the class.
21-21: LGTM!The addition of
_sensitivities_implementedasTrueis correct and indicates that sensitivity analysis features are now supported within this class.
22-22: LGTM!The addition of
_parameter_boundsas[[ -_np.pi / 2, _np.pi / 2 ]]is correct and provides explicit limits for the parameters.
23-23: LGTM!The addition of
_parameters_shapeas(1, 1)is correct and aids in maintaining consistency in parameter handling throughout the class.
54-63: LGTM!The modification of the
create_tilemethod to incorporateangle_boundsfrom_parameter_boundsenhances maintainability and clarity by centralizing the bounds in a single attribute.
59-63: LGTM!The improved error message in the
create_tilemethod dynamically incorporates the actual bounds fromangle_bounds, providing more informative feedback to the user.
54-63: LGTM!The overall changes in the
create_tilemethod reflect a shift towards a more structured and maintainable approach to parameter management and validation.splinepy/microstructure/tiles/cube_void.py (5)
28-28: LGTM!The addition of
_sensitivities_implementedasTrueis correct and indicates that sensitivity analysis features are now supported within this class.
31-36: LGTM!The addition of
_parameter_boundsas[[0.0, 1.0], [0.0, 1.0], [-_np.pi / 2, _np.pi / 2], [-_np.pi / 2, _np.pi / 2]]is correct and provides explicit limits for the parameters.
37-37: LGTM!The addition of
_parameters_shapeas(1, 4)is correct and aids in maintaining consistency in parameter handling throughout the class.
104-106: LGTM!The modification of the
create_tilemethod to incorporate_parameters_shapefor default initialization ofparametersenhances flexibility and maintainability by utilizing_parameters_shapeinstead of hardcoding the shape directly.
104-106: LGTM!The overall changes in the
create_tilemethod reflect a shift towards a more robust and flexible design, facilitating better parameter management.examples/show_microstructures.py (3)
349-349: LGTM!The parameter name "seperator_distance" has been corrected to "separator_distance". This change improves clarity and consistency in the codebase.
355-355: LGTM!The parameter name "seperator_distance" has been corrected to "separator_distance". This change improves clarity and consistency in the codebase.
368-368: LGTM!The parameter name "seperator_distance" has been corrected to "separator_distance". This change improves clarity and consistency in the codebase.
splinepy/microstructure/tiles/double_lattice.py (4)
23-23: LGTM!The attribute
_sensitivities_implementedhas been added to theDoubleLatticeclass. This indicates that sensitivity analysis features are now supported within the class.
24-24: LGTM!The attribute
_parameter_boundshas been added to theDoubleLatticeclass. This specifies the bounds for parameters used in the double lattice calculations.
25-25: LGTM!The attribute
_parameters_shapehas been added to theDoubleLatticeclass. This defines the expected shape of the parameters used in the class.
50-50: LGTM!The docstring of the
create_tilemethod has been modified for clarity. This improves the clarity and grammatical accuracy of the documentation.tests/test_microstructure.py (7)
1-10: LGTM!The imports and constants are necessary for the tests.
19-26: LGTM!The function
check_control_pointscorrectly checks if all control points lie within the unit square/cube.
29-46: LGTM!The function
make_bounds_feasiblecorrectly converts bounds to an open set.
49-97: LGTM!The function
test_tile_classcorrectly checks if all tile classes have the appropriate members and functions.
99-129: LGTM!The function
test_tile_boundscorrectly tests if the tile is still in the unit cube at the bounds.
132-177: LGTM!The function
test_tile_closurecorrectly checks if closing tiles also lie in the unit cube.
179-281: LGTM!The function
test_tile_derivativescorrectly tests the correctness of the tile derivatives using Finite Differences.splinepy/microstructure/tiles/hollow_cube.py (4)
31-31: LGTM!The
_sensitivities_implementedattribute is correctly added to indicate that sensitivity analysis is supported.
32-32: LGTM!The
_parameter_boundsattribute is correctly added to define the valid range for parameters.
33-33: LGTM!The
_parameters_shapeattribute is correctly added to specify the expected shape of the parameters.
71-72: LGTM!The parameter validation logic has been correctly streamlined to use a single condition with NumPy's array operations.
splinepy/microstructure/tiles/ellips_v_oid.py (3)
30-30: LGTM!The
_sensitivities_implementedattribute is correctly added to indicate that sensitivity analysis is supported.
34-39: LGTM!The
_parameter_boundsattribute is correctly added to define the valid range for parameters.
40-40: LGTM!The
_parameters_shapeattribute is correctly added to specify the expected shape of the parameters.splinepy/microstructure/tiles/snappy.py (6)
23-23: LGTM!The
_sensitivities_implementedattribute is correctly added to indicate that sensitivity analysis is supported.
24-24: LGTM!The
_closure_directionsattribute is correctly added to define the closure directions.
25-25: LGTM!The
_parameter_boundsattribute is correctly added to define the valid range for parameters.
26-26: LGTM!The
_parameters_shapeattribute is correctly added to specify the expected shape of the parameters.
68-70: LGTM!The logic has been correctly adjusted to conditionally check parameters only if they are not
None.
371-371: Fix the typo in the error message.The error message for unimplemented parameterization has been corrected from "Parametriazation" to "Parametrization."
Apply this diff to fix the typo in the error message:
- "Parametriazation is not implemented for this tile yet" + "Parametrization is not implemented for this tile yet"Likely invalid or redundant comment.
splinepy/microstructure/tiles/cross_2d.py (5)
29-29: LGTM!The addition of
_sensitivities_implementedattribute is clear and indicates that sensitivity analysis features are now supported.
30-30: LGTM!The addition of
_closure_directionsattribute enhances the configurability of the tile's behavior in a two-dimensional space.
31-33: LGTM!The addition of
_parameter_boundsattribute ensures that the parameters are constrained to a specific range, improving robustness.
34-34: LGTM!The addition of
_parameters_shapeattribute facilitates validation and error checking.
427-428: LGTM!The updated error message provides clearer guidance on the acceptable range for the
center_expansionparameter.splinepy/microstructure/tiles/hollow_octagon_extrude.py (11)
21-21: LGTM!The addition of
_sensitivities_implementedattribute is clear and indicates that sensitivity analysis features are now supported.
23-23: LGTM!The addition of
_closure_directionsattribute enhances the configurability of the tile's behavior in a three-dimensional space.
24-24: LGTM!The addition of
_parameter_boundsattribute ensures that the parameters are constrained to a specific range, improving robustness.
25-25: LGTM!The addition of
_parameters_shapeattribute facilitates validation and error checking.
74-78: LGTM!The handling of
parameter_sensitivitiesincreate_tilemethod now allows the method to process sensitivity information, enhancing its flexibility and usability.
84-87: LGTM!The validation for the
v_h_voidparameter has been modified to allow a lower bound of 0.0 instead of 0.01, broadening the acceptable range for this parameter.
89-245: LGTM!The logic for generating spline control points in
create_tilemethod now constructs control points for splines in a loop that iterates over the number of derivatives, allowing for the creation of derivative-specific splines.
247-247: LGTM!The return statement of
create_tilemethod now returns both the primary splines and the derivatives, providing a more comprehensive output.
249-249: LGTM!The renaming of
closing_tilemethod to_closing_tileclarifies the intended usage context of the method.
76-80: LGTM!The handling of
parameter_sensitivitiesin_closing_tilemethod now allows the method to process sensitivity information, enhancing its flexibility and usability.
83-87: LGTM!The validation for the
v_h_voidparameter has been modified to allow a lower bound of 0.0 instead of 0.01, broadening the acceptable range for this parameter.splinepy/microstructure/tiles/hollow_octagon.py (8)
21-21: LGTM!The addition of
_sensitivities_implementedattribute is clear and indicates that sensitivity analysis features are now supported.
22-22: LGTM!The addition of
_closure_directionsattribute enhances the configurability of the tile's behavior in a two-dimensional space.
23-23: LGTM!The addition of
_parameter_boundsattribute ensures that the parameters are constrained to a specific range, improving robustness.
24-24: LGTM!The addition of
_parameters_shapeattribute facilitates validation and error checking.
76-80: LGTM!The handling of
parameter_sensitivitiesin_closing_tilemethod now allows the method to process sensitivity information, enhancing its flexibility and usability.
83-87: LGTM!The validation for the
v_h_voidparameter has been modified to allow a lower bound of 0.0 instead of 0.01, broadening the acceptable range for this parameter.
89-418: LGTM!The logic for generating spline control points in
_closing_tilemethod now constructs control points for splines in a loop that iterates over the number of derivatives, allowing for the creation of derivative-specific splines.
420-420: LGTM!The return statement of
_closing_tilemethod now returns both the primary splines and the derivatives, providing a more comprehensive output.splinepy/microstructure/tiles/cross_3d_linear.py (4)
31-31: LGTM!The addition of
_sensitivities_implementedattribute is clear and indicates that the class supports sensitivity analysis.
32-32: LGTM!The addition of
_closure_directionsattribute enhances the configurability of the class by defining valid closure directions.
33-35: LGTM!The addition of
_parameter_boundsattribute ensures that the parameters used in the class are within the specified bounds, enhancing robustness.
36-36: LGTM!The addition of
_parameters_shapeattribute ensures that the parameters used in the class have the correct shape, enhancing robustness.splinepy/microstructure/tiles/cross_3d.py (5)
31-31: LGTM!The addition of
_sensitivities_implementedattribute is clear and indicates that the class supports sensitivity analysis.
32-32: LGTM!The addition of
_closure_directionsattribute enhances the configurability of the class by defining valid closure directions.
33-35: LGTM!The addition of
_parameter_boundsattribute ensures that the parameters used in the class are within the specified bounds, enhancing robustness.
36-36: LGTM!The addition of
_parameters_shapeattribute ensures that the parameters used in the class have the correct shape, enhancing robustness.
566-569: LGTM!The addition of closure validation in the
create_tilemethod enhances error handling by ensuring that only supported closure types are processed.splinepy/microstructure/microstructure.py (2)
12-12: LGTM!The correction of the class-level docstring enhances the clarity and professionalism of the documentation.
21-21: LGTM!The correction of the constructor's docstring enhances the clarity and professionalism of the documentation.
splinepy/microstructure/tiles/inverse_cross_3d.py (3)
35-39: LGTM!The class attributes are correctly defined and initialized.
893-893: LGTM! But verify the function usage in the codebase.The function is correctly implemented. However, ensure that all function calls to
create_tilematch the new signature.The code changes are approved.
Run the following script to verify the function usage:
48-48: LGTM! But verify the function usage in the codebase.The function is correctly implemented. However, ensure that all function calls to
_closing_tilematch the new signature.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Function usage is consistent with the updated signature.
The
_closing_tilefunction insplinepy/microstructure/tiles/inverse_cross_3d.pyis correctly defined and used with theseparator_distanceparameter. No issues were found regarding its usage in this context.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `_closing_tile` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'_closing_tile'Length of output: 68
Script:
#!/bin/bash # Description: Verify all function calls to `_closing_tile` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg '_closing_tile' --glob '*.py' -A 5Length of output: 9016
|
The current status is:
|
clemens-fricke
left a comment
clemens-fricke
left a comment
There was a problem hiding this comment.
Is there still progress planed on this? This looks very interesting, thanks for the work you have already done.
|
|
||
| spline_list = [] | ||
|
|
||
| if closure == "x_min": |
There was a problem hiding this comment.
I kind if wish that there is a less verbose way of defining these. But I am not sure. If it is easily doable while keeping readability high.
|
I just checked the tests on my local laptop. My implemented tests are flaky due to the random points: most of the time, all tests pass, but sometimes there is one failing test. I think sticking to a fixed random seed might be a better idea, for replicability |
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (1)
splinepy/microstructure/tiles/snappy.py (1)
71-73:⚠️ Potential issueRemove redundant parameter check.
Based on the past review comments and the fact that parameters are not implemented, this check is redundant and should be removed.
- # TODO: parameters are not implemented, therefore do not check params - if parameters is not None: - self.check_params(parameters)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
splinepy/microstructure/microstructure.py(5 hunks)splinepy/microstructure/tiles/chi.py(3 hunks)splinepy/microstructure/tiles/cube_void.py(3 hunks)splinepy/microstructure/tiles/ellips_v_oid.py(3 hunks)splinepy/microstructure/tiles/hollow_octagon_extrude.py(3 hunks)splinepy/microstructure/tiles/snappy.py(5 hunks)splinepy/microstructure/tiles/tile_base.py(0 hunks)tests/conftest.py(2 hunks)tests/test_knot_vectors.py(1 hunks)tests/test_microstructure.py(1 hunks)tests/test_microtiles.py(1 hunks)
💤 Files with no reviewable changes (1)
- splinepy/microstructure/tiles/tile_base.py
🔇 Additional comments (8)
splinepy/microstructure/tiles/hollow_octagon_extrude.py (1)
75-80: Verify the dimensions ofparameter_sensitivities.Accessing
parameter_sensitivities.shape[2]assumes thatparameter_sensitivitiesis at least three-dimensional. Ensure that the input array has the expected dimensions to prevent potentialIndexError.tests/test_knot_vectors.py (1)
32-32: Update assertion to use.numpy()method onspl_kv.The change correctly accesses the NumPy array representation of
spl_kvusing.numpy(), ensuring the assertion compares the correct data structures.splinepy/microstructure/tiles/cube_void.py (1)
28-37: 🧹 Nitpick (assertive)Address the TODO comment about parameter bounds.
The TODO comment indicates that parameter bounds and checks need improvement to ensure the parametrization lies within the unit cube.
Do you want me to help implement smarter parameter bounds and validation checks?
tests/conftest.py (1)
18-18: LGTM! Fixed seed ensures reproducible tests.Using a fixed seed for the random number generator addresses the test flakiness concerns and ensures reproducible test results.
splinepy/microstructure/tiles/ellips_v_oid.py (2)
97-97: Verify the rotation matrix derivative implementation.The change in the y-axis rotation matrix derivative needs verification to ensure mathematical correctness.
Let's verify this against other implementations:
✅ Verification successful
The y-axis rotation matrix derivative implementation is mathematically correct.
The implementation correctly represents the derivative of the y-axis rotation matrix with respect to the angle, where:
- The original rotation matrix entries cos(θ) are correctly differentiated to -sin(θ)
- The original rotation matrix entries sin(θ) are correctly differentiated to cos(θ)
- The implementation is consistent across multiple files in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find other rotation matrix implementations for comparison # Search for other rotation matrix implementations ast-grep --pattern $'def _rotation_matrix_.*_deriv\(.*\).*: $$$ return.*'Length of output: 78
Script:
#!/bin/bash # Search for rotation-related implementations and tests rg -l "rotation" --type py rg "rotation.*matrix" -A 5 -B 5 --type pyLength of output: 14401
33-43: Verify parameter bounds for ellipsoid containment.The parameter bounds need verification to ensure they're sufficient for keeping the ellipsoid within the unit cube. The TODO comment indicates this is still an open issue.
Let's analyze the parameter space:
✅ Verification successful
Parameter bounds are correct but control points may exceed unit cube bounds
The parameter bounds ([0,1] for radii, [-π/2,π/2] for rotations) are mathematically correct for the ellipsoid transformation. However, as documented in the code, while these bounds ensure the final geometry stays within the unit cube, the control points may still lie outside it. This is a known limitation that's already being tracked.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances where these parameter bounds are used to verify containment # Search for parameter validation rg -A 5 "_parameter_bounds.*\[.*0\.0.*1\.0" # Search for related tests rg -A 10 "test.*ellips.*bound"Length of output: 78
Script:
#!/bin/bash # Find the complete class implementation ast-grep --pattern 'class $_ { $$$ _parameter_bounds = [ $$$ ] $$$ }' # Search for ellipsoid tests and implementations rg -l "ellips.*v.*oid" # Search for parameter validation rg -B 5 -A 10 "def validate.*param"Length of output: 234
Script:
#!/bin/bash # View the implementation file cat splinepy/microstructure/tiles/ellips_v_oid.py # Search for tests with different patterns rg -l "test.*ellips" rg -l "test.*void" # Search for parameter usage rg -A 5 "def.*param" splinepy/microstructure/tiles/ellips_v_oid.pyLength of output: 12775
splinepy/microstructure/microstructure.py (2)
430-431: LGTM! Default parameter values updated.The default values for
knot_span_wiseandmacro_sensitivitiesare now explicitly set to their most common use case.
458-463: LGTM! Added type validation for boolean parameters.The new assertions improve parameter validation by ensuring boolean types.
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (1)
splinepy/microstructure/tiles/chi.py (1)
Line range hint
96-96: Correct the typo in the method namesensitivites_implemented.The method name
sensitivites_implementedhas a typo; it should besensitivities_implemented. This will prevent correct property access and may lead to attribute errors.Apply this diff to fix the typo:
- def sensitivites_implemented(cls): + def sensitivities_implemented(cls):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
splinepy/microstructure/tiles/chi.py(3 hunks)splinepy/microstructure/tiles/cross_2d.py(3 hunks)splinepy/microstructure/tiles/cross_3d.py(3 hunks)splinepy/microstructure/tiles/cross_3d_linear.py(3 hunks)splinepy/microstructure/tiles/cube_void.py(3 hunks)splinepy/microstructure/tiles/double_lattice.py(2 hunks)splinepy/microstructure/tiles/ellips_v_oid.py(4 hunks)splinepy/microstructure/tiles/hollow_cube.py(2 hunks)splinepy/microstructure/tiles/hollow_octagon.py(5 hunks)splinepy/microstructure/tiles/hollow_octagon_extrude.py(3 hunks)splinepy/microstructure/tiles/snappy.py(6 hunks)splinepy/microstructure/tiles/tile_base.py(5 hunks)tests/conftest.py(2 hunks)tests/test_microstructure.py(1 hunks)tests/test_microtiles.py(1 hunks)
🧰 Additional context used
📓 Learnings (7)
splinepy/microstructure/tiles/snappy.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:26-29
Timestamp: 2025-01-16T16:38:32.816Z
Learning: In tile classes where parameters are not implemented (like Snappy), empty parameter bounds (`_parameter_bounds = []`) and shape (`_parameters_shape = ()`) are appropriate, as there are no parameters to constrain or shape.
tests/test_microstructure.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microstructure.py:51-52
Timestamp: 2025-01-16T14:31:31.072Z
Learning: When testing closing faces in microstructure tests, boundary ID assertions are not needed as the test only uses specific boundary IDs (2 and 3) as temporary identifiers for min and max boundaries to verify surface areas.
splinepy/microstructure/tiles/hollow_octagon.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/hollow_octagon_extrude.py:94-98
Timestamp: 2025-01-16T14:27:27.307Z
Learning: Parameter bounds validation for tile parameters has been moved to the TileBase class to centralize the validation logic and reduce code duplication across individual tile classes.
splinepy/microstructure/tiles/cube_void.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cube_void.py:67-67
Timestamp: 2025-01-16T16:08:24.359Z
Learning: In the y-axis rotation matrix derivative, the middle row is [0, 0, 0] because it's the derivative of [0, 1, 0] with respect to the rotation angle. This follows from the fact that y-coordinates remain unchanged during rotation around y-axis, and the derivative of constants is zero.
splinepy/microstructure/tiles/chi.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/chi.py:86-92
Timestamp: 2025-01-16T14:27:59.957Z
Learning: In the Chi class's `create_tile` method, the calculation `dalpha * -r * _np.sin(alpha)` is structured to emphasize that it's the derivative of `r * cos(alpha)` with respect to alpha, where the negative sign placement reflects the chain rule application.
tests/test_microtiles.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:157-159
Timestamp: 2025-01-16T16:28:22.607Z
Learning: In splinepy's microstructure tests, $C_0$-continuity at closure boundaries is implicitly verified through surface integral tests. If the surface integral equals 1, it indicates no gaps or overlaps exist between patches at the closure boundary.
splinepy/microstructure/tiles/hollow_octagon_extrude.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/hollow_octagon_extrude.py:94-98
Timestamp: 2025-01-16T14:27:27.307Z
Learning: Parameter bounds validation for tile parameters has been moved to the TileBase class to centralize the validation logic and reduce code duplication across individual tile classes.
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: build_and_tests (3.12, windows-latest)
- GitHub Check: build_and_tests (3.12, macos-latest)
- GitHub Check: build_and_tests (3.12, ubuntu-20.04)
- GitHub Check: build_and_tests (3.11, windows-latest)
- GitHub Check: build_and_tests (3.11, macos-latest)
- GitHub Check: build_and_tests (3.11, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-latest)
- GitHub Check: build_and_tests (3.10, ubuntu-20.04)
- GitHub Check: build_and_tests (3.9, windows-latest)
- GitHub Check: build_and_tests (3.9, ubuntu-20.04)
- GitHub Check: build_and_tests (3.8, windows-latest)
- GitHub Check: build_and_tests (3.8, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-14)
- GitHub Check: build_and_tests (3.10, macos-13)
- GitHub Check: minimal_explicit_build_and_docs
- GitHub Check: build_and_tests (3.10, ubuntu-latest)
🔇 Additional comments (31)
splinepy/microstructure/tiles/hollow_cube.py (1)
61-64: Ensure input validation in_process_inputmethodThe previous input handling and validation logic in
create_tilehas been replaced with a call toself._process_input. Please verify that_process_inputadequately validatesparametersandparameter_sensitivities, including checking value ranges and shapes, and raises appropriate exceptions when necessary to prevent runtime errors.splinepy/microstructure/tiles/snappy.py (3)
26-29: LGTM! Class attributes are correctly defined.The new attributes accurately reflect the current state of the class:
_sensitivities_implemented = Falsealigns with the unimplemented parameter sensitivities_closure_directionscorrectly specifies y-axis closures- Empty
_parameter_boundsand_parameters_shapeare appropriate since parameters are not implemented
356-358: LGTM! Parameter validation and error message improvements.The changes enhance the code quality:
- Added proper bounds checking for contact_length parameter
- Fixed the typo in the error message
Also applies to: 375-375
11-12: 🧹 Nitpick (assertive)Enhance TODO comment clarity.
The TODO comment could be more specific about what's needed to implement parameters and sensitivities.
- # TODO: currently the tile parameters are not implemented as the parameters variable - therefore, no parameter sensitivities are calculated + # TODO: Parameter implementation needed: + # 1. Define parameter array structure for geometric properties (a, b, c, r) + # 2. Implement parameter validation in create_tile and _closing_tile + # 3. Add parameter sensitivity calculations for optimizationLikely invalid or redundant comment.
splinepy/microstructure/tiles/cross_2d.py (5)
29-35: Initialization of new class attributes aligns with the updated parameter handling.The addition of
_sensitivities_implemented,_closure_directions,_parameter_bounds,_parameters_shape, and_default_parameter_valueenhances the class's ability to handle parameters and sensitivities consistently. These attributes are correctly defined and are consistent with the modifications made in other tile classes.
76-79: Consolidation of parameter processing using_process_inputmethod improves maintainability.Replacing the previous parameter handling logic with a call to
self._process_inputcentralizes parameter and sensitivity processing. This refactoring promotes code reuse and reduces redundancy, enhancing code clarity and maintainability.
81-86: Validation of custom parameters using_check_custom_parameterensures robustness.The use of
_check_custom_parameterforboundary_widthandfilling_heighteffectively validates custom parameters within their specified bounds. This approach standardizes parameter validation and helps prevent invalid inputs.
405-407: Correct validation ofcenter_expansionparameter enhances input reliability.Validating
center_expansionusing_check_custom_parameterensures that it falls within the acceptable range of 0.5 to 1.5. This check is crucial to prevent potential errors during tile creation due to invalid expansion factors.
409-412: Consistent use of_process_inputincreate_tilemethod standardizes parameter handling.By utilizing
self._process_inputfor parameter and sensitivity processing in thecreate_tilemethod, the code achieves consistency across methods. This enhances code maintainability and reduces the likelihood of discrepancies in parameter handling.splinepy/microstructure/tiles/double_lattice.py (3)
51-51: LGTM! Grammar fix improves clarity.The correction of "tiles boundary" to "tile's boundary" properly fixes the possessive form.
57-64: LGTM! Improved parameter validation and processing.The refactoring enhances code maintainability by:
- Centralizing parameter validation through
_check_custom_parameter- Consolidating parameter and sensitivity processing through
_process_input
57-64: Verify test coverage for parameter validation and sensitivity analysis.Given that this PR focuses on testing microstructure and microtiles, let's ensure adequate test coverage exists for:
- Parameter bounds validation
- Sensitivity derivatives calculation
- Edge cases in parameter processing
Let's check for existing tests:
✅ Verification successful
Test coverage is comprehensive and well-implemented
The codebase already includes thorough test coverage for parameter validation and sensitivity analysis:
- Parameter bounds validation is tested through base class validation
- Sensitivity derivatives are rigorously tested using finite difference validation in
test_tile_derivatives()- Integration tests verify the impact on the full microstructure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of DoubleLattice parameter validation and sensitivity analysis # Look for test files related to DoubleLattice fd -e py -E "test.*double.*lattice.*" -E "double.*lattice.*test.*" # Search for specific test cases rg -l "DoubleLattice.*test|test.*DoubleLattice" -t py # Look for sensitivity test patterns rg "test.*sensitivity|sensitivity.*test" -t py -A 5Length of output: 3539
Script:
#!/bin/bash # Check content of microstructure test files for DoubleLattice tests rg -A 10 "DoubleLattice" tests/test_microtiles.py tests/test_microstructure.py # Check tile base class tests for parameter validation rg -A 10 "_check_custom_parameter|_process_input" tests/test_microtiles.py tests/test_microstructure.py # Look for sensitivity-related tests in microstructure files rg -A 10 "parameter_sensitivities|derivatives" tests/test_microtiles.py tests/test_microstructure.pyLength of output: 5049
tests/test_microstructure.py (1)
157-161: 🧹 Nitpick (assertive)Add shape assertions for derivatives.
Before comparing derivative values, verify that the shapes match the expected dimensions.
Apply this diff to add shape assertions:
deriv_evaluations = [ patch.evaluate(eval_points) for patch in deriv_orig.patches ] + # Verify derivative shapes + for patch_deriv in deriv_evaluations: + assert patch_deriv.shape[1] == dim, ( + f"Derivative shape mismatch. Expected {dim} dimensions, " + f"got {patch_deriv.shape[1]}" + )Likely invalid or redundant comment.
tests/conftest.py (3)
18-18: LGTM! Good choice using a fixed seed.Using a fixed seed (0) for the random number generator ensures test reproducibility.
44-55: LGTM! Well-documented trade-off.The docstring clearly explains why 1e-7 was chosen as a compromise between truncation and round-off errors in finite difference calculations.
58-65: 🧹 Nitpick (assertive)Improve docstring to explain the trade-off.
The docstring could better explain why 10 test points were chosen.
Apply this diff to improve the docstring:
""" Number of random testing points (in parametric domain) - The number 10 is arbitrary and should ensure to have good test coverage + The value 10 is chosen as a balance between: + - Having enough points to ensure good test coverage + - Keeping test execution time reasonable """Likely invalid or redundant comment.
tests/test_microtiles.py (1)
120-121: 🧹 Nitpick (assertive)Add logging for skipped tiles.
Consider adding logging to track which tile classes are being skipped.
Apply this diff to add logging:
if tile_class in skip_tiles: + print(f"Skipping bounds test for {tile_class.__name__} as it's known to fail") returnLikely invalid or redundant comment.
splinepy/microstructure/tiles/chi.py (1)
19-24: Verify the change in_evaluation_pointsdimensionality.The
_evaluation_pointsattribute has been changed from a 2D array[[0.5, 0.5]]to a 1D array[[0.5]]. Please ensure that this change aligns with the expected dimensionality for this tile and that dependent methods handle this change correctly to prevent any unintended side effects.splinepy/microstructure/tiles/tile_base.py (2)
198-200: 🛠️ Refactor suggestionClarify parameter bounds inclusivity and adjust comparisons if necessary.
The parameter bounds are currently checked using exclusive comparisons (
>and<). If parameters equal to the bounds should be considered valid, please use inclusive comparisons (>=and<=). This will ensure that boundary values are correctly accepted or rejected as intended.Apply this diff to adjust the comparisons:
- within_bounds = (parameters.ravel() > lower_bounds) & ( - parameters.ravel() < upper_bounds + within_bounds = (parameters.ravel() >= lower_bounds) & ( + parameters.ravel() <= upper_bounds )
325-328: 🧹 Nitpick (assertive)Ensure parameter bounds check is consistent with expected inclusivity.
The current check for custom parameters uses exclusive bounds:
if not ((value > min_bound) and (value < max_bound)):If parameters equal to
min_boundormax_boundare acceptable, consider using inclusive comparisons (>=and<=) to accurately reflect the valid range.Apply this diff to adjust the condition:
- if not ((value > min_bound) and (value < max_bound)): + if not ((value >= min_bound) and (value <= max_bound)):splinepy/microstructure/tiles/ellips_v_oid.py (2)
98-98: Confirm the correction in_rotation_matrix_y_deriv.The return statement in
_rotation_matrix_y_derivhas been changed, updating the middle row from[0, 1, 0]to[0, 0, 0]. This aligns with the mathematical derivative of the rotation matrix around the y-axis. Please confirm that this change is intentional and that it correctly affects derivative calculations.
Line range hint
325-328: Ensure parameter bounds check reflects intended inclusivity.The check for parameter bounds currently uses exclusive comparisons:
if not ((value > min_bound) and (value < max_bound)):If the boundary values are valid, please adjust to inclusive comparisons (
>=and<=).Apply this diff to adjust the condition:
- if not ((value > min_bound) and (value < max_bound)): + if not ((value >= min_bound) and (value <= max_bound)):splinepy/microstructure/tiles/hollow_octagon.py (2)
21-25: Class attributes initialization is appropriateThe addition of
_sensitivities_implemented,_closure_directions,_parameter_bounds,_parameters_shape, and_default_parameter_valueenhances the configurability and consistency of theHollowOctagonclass.
59-62: Use of_process_inputmethod improves parameter handlingImplementing the
_process_inputmethod for processing parameters and sensitivities centralizes input validation and reduces code duplication.splinepy/microstructure/tiles/hollow_octagon_extrude.py (3)
21-25: Class attributes added correctlyThe new class attributes
_sensitivities_implemented,_closure_directions,_parameter_bounds,_parameters_shape, and_default_parameter_valueenhance the tile's flexibility and align with standard practices.
32-32: Addition ofclosureparameter tocreate_tilemethodIncluding the
closureparameter in thecreate_tilemethod signature allows for more versatile tile creation and supports closure handling.
59-65: Parameter checks and input processing are well-implementedUsing
_check_custom_parameterforcontact_lengthvalidation and_process_inputfor parameter handling improves code maintainability and consistency across tile classes.splinepy/microstructure/tiles/cube_void.py (3)
28-38: Enhancement of parameter configuration with new class attributesThe addition of
_sensitivities_implemented,_parameter_bounds,_parameters_shape, and_default_parameter_valueimproves configurability and aligns theCubeVoidclass with other tile classes.
68-68: Correct computation of the rotation matrix derivativeThe update to the
_rotation_matrix_y_derivmethod accurately reflects the derivative of the rotation matrix around the y-axis. The second row being[0, 0, 0]correctly represents the zero derivative of constants.
105-108: Utilization of_process_inputfor parameter handlingEmploying the
_process_inputmethod for parameters and sensitivities streamlines input processing and enhances code consistency with other tile classes.splinepy/microstructure/tiles/cross_3d.py (1)
531-534: LGTM! Improved closure validation.The explicit validation against
_closure_directionsimproves code robustness and provides a clearer error message.
| _sensitivities_implemented = True | ||
| _parameter_bounds = [[0.0, 1 / (2 * (1 + _np.sqrt(2)))]] * 2 | ||
| _parameters_shape = (1, 2) | ||
| _default_parameter_value = 0.1 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider documenting the parameter bounds formula.
The new class attributes effectively support sensitivity analysis and parameter validation. However, the mathematical formula used for parameter bounds (1 / (2 * (1 + _np.sqrt(2)))) would benefit from a brief explanation of its derivation and significance.
Consider adding a comment explaining how this bound was derived and why it's critical for the double lattice structure.
There was a problem hiding this comment.
@mkofler96 do you have any idea, why the bounds of the parameters are set to this value? If so and you want to, can you add the formula for it? If not, we can probably leave it for now.
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!
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (3)
splinepy/microstructure/tiles/double_lattice.py (1)
24-24: 🧹 Nitpick (assertive)Consider documenting the derivation of
_parameter_boundsThe parameter bounds are set using the formula
1 / (2 * (1 + _np.sqrt(2))). Adding a comment to explain the derivation and significance of this formula would enhance code readability and assist future maintainers.splinepy/microstructure/tiles/hollow_octagon.py (1)
69-399: 🧹 Nitpick (assertive)Consider further refactoring to eliminate code duplication.
The loops and computations within the
_closing_tileandcreate_tilemethods share considerable code. Even after moving some logic toTileBase, there is still duplicated code between these methods. Consider extracting shared logic into additional helper methods to further reduce duplication and enhance maintainability.Also applies to: 455-565
splinepy/microstructure/tiles/hollow_octagon_extrude.py (1)
81-239: 🧹 Nitpick (assertive)Consider refactoring duplicated code between
create_tileand_closing_tile.The
_closing_tileandcreate_tilemethods have substantial code duplication in the loops and computations for generating splines. To improve maintainability, consider extracting shared logic into helper methods, possibly in theTileBaseclass or within this class.Also applies to: 284-1254
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
splinepy/microstructure/tiles/cross_2d.py(3 hunks)splinepy/microstructure/tiles/cross_3d.py(3 hunks)splinepy/microstructure/tiles/cross_3d_linear.py(3 hunks)splinepy/microstructure/tiles/double_lattice.py(2 hunks)splinepy/microstructure/tiles/hollow_octagon.py(5 hunks)splinepy/microstructure/tiles/hollow_octagon_extrude.py(3 hunks)splinepy/microstructure/tiles/snappy.py(6 hunks)splinepy/microstructure/tiles/tile_base.py(5 hunks)tests/test_microstructure.py(1 hunks)tests/test_microtiles.py(1 hunks)
🧰 Additional context used
📓 Learnings (9)
tests/test_microstructure.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microstructure.py:51-52
Timestamp: 2025-01-16T14:31:31.072Z
Learning: When testing closing faces in microstructure tests, boundary ID assertions are not needed as the test only uses specific boundary IDs (2 and 3) as temporary identifiers for min and max boundaries to verify surface areas.
splinepy/microstructure/tiles/snappy.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:26-29
Timestamp: 2025-01-16T16:38:32.816Z
Learning: In tile classes where parameters are not implemented (like Snappy), empty parameter bounds (`_parameter_bounds = []`) and shape (`_parameters_shape = ()`) are appropriate, as there are no parameters to constrain or shape.
splinepy/microstructure/tiles/cross_2d.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/cross_3d.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:78-88
Timestamp: 2025-01-17T16:30:34.847Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept within individual tile classes rather than being moved to TileBase, as each tile can have different validation requirements.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
splinepy/microstructure/tiles/cross_3d_linear.py (2)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
tests/test_microtiles.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:157-159
Timestamp: 2025-01-16T16:28:22.607Z
Learning: In splinepy's microstructure tests, $C_0$-continuity at closure boundaries is implicitly verified through surface integral tests. If the surface integral equals 1, it indicates no gaps or overlaps exist between patches at the closure boundary.
splinepy/microstructure/tiles/hollow_octagon_extrude.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/hollow_octagon_extrude.py:94-98
Timestamp: 2025-01-16T14:27:27.307Z
Learning: Parameter bounds validation for tile parameters has been moved to the TileBase class to centralize the validation logic and reduce code duplication across individual tile classes.
splinepy/microstructure/tiles/hollow_octagon.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/hollow_octagon_extrude.py:94-98
Timestamp: 2025-01-16T14:27:27.307Z
Learning: Parameter bounds validation for tile parameters has been moved to the TileBase class to centralize the validation logic and reduce code duplication across individual tile classes.
splinepy/microstructure/tiles/tile_base.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: In splinepy's TileBase, parameter shape validation is handled through two mechanisms:
1. Test-time validation of default_parameter_value dimensions in test_microtiles.py
2. Runtime validation of final parameter shape in TileBase.check_params()
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: Parameter shape validation in splinepy's TileBase is implemented through multiple layers:
1. Class-level validation in test_tile_class ensures _parameters_shape exists and is a tuple
2. Runtime validation in TileBase.check_params() verifies parameter dimensions match (evaluation_points.shape[0], n_info_per_eval_point)
3. Comprehensive testing in test_tile_bounds validates both default and boundary parameter cases
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build_and_tests (3.12, windows-latest)
- GitHub Check: build_and_tests (3.12, macos-latest)
- GitHub Check: build_and_tests (3.12, ubuntu-20.04)
- GitHub Check: build_and_tests (3.11, windows-latest)
- GitHub Check: build_and_tests (3.11, macos-latest)
- GitHub Check: build_and_tests (3.11, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-latest)
- GitHub Check: build_and_tests (3.10, ubuntu-20.04)
- GitHub Check: build_and_tests (3.9, windows-latest)
- GitHub Check: build_and_tests (3.9, ubuntu-20.04)
- GitHub Check: build_and_tests (3.8, windows-latest)
- GitHub Check: build_and_tests (3.8, macos-latest)
- GitHub Check: build_and_tests (3.8, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-13)
- GitHub Check: build_and_tests (3.10, ubuntu-latest)
🔇 Additional comments (30)
splinepy/microstructure/tiles/snappy.py (7)
26-29: Class attributes are correctly defined.The empty parameter bounds and shape are appropriate since parameters are not implemented in this tile class, as confirmed by the retrieved learning from markriegler.
31-31: Verify contact length bounds.The contact length bounds of [0.0, 0.49] need verification against the geometric constraints:
- Must be positive (validated in create_tile)
- Must satisfy
r + contact_length < 0.5(validated in create_tile)
302-303: Return statement is correctly positioned.The return statement is now outside the conditional block, ensuring it always returns a tuple of (spline_list, None), which is consistent with the method's contract.
356-358: Parameter validation is properly encapsulated.The contact length validation has been moved to a dedicated method, improving code organization and reusability.
375-375: Fixed typo in error message.The error message now correctly spells "Parametrization" instead of "Parametriazation".
73-73: 🧹 Nitpick (assertive)Remove redundant TODO comment.
The TODO comment about not checking parameters is redundant since:
- Parameters are already documented as not implemented in the class docstring
- The code no longer performs parameter validation at this point
- # TODO: parameters are not implemented, therefore do not check paramsLikely invalid or redundant comment.
11-12: 🧹 Nitpick (assertive)Improve TODO comment clarity.
The TODO comment should be more specific about why parameters aren't implemented and what's needed to implement them.
- # TODO: currently the tile parameters are not implemented as the parameters variable - therefore, no parameter sensitivities are calculated + # TODO: Parameters and sensitivities not implemented: + # - Parameters are not used in the tile's geometry definition + # - Consequently, parameter sensitivities cannot be calculatedLikely invalid or redundant comment.
splinepy/microstructure/tiles/double_lattice.py (3)
23-28: Addition of new class attributes enhances parameter handlingThe introduction of
_sensitivities_implemented,_parameter_bounds,_parameters_shape,_default_parameter_value, and_CONTACT_LENGTH_BOUNDSimproves the class's ability to handle sensitivities and parameter validations effectively.
59-61: Proper validation ofcontact_lengthparameterUsing
self._check_custom_parameterto validatecontact_lengthagainst_CONTACT_LENGTH_BOUNDSensures that thecontact_lengthstays within the intended bounds of[0.0, 1.0]. This enhances input validation and robustness of the method.
63-66: Refactored parameter processing improves code maintainabilityThe use of
_process_inputto handleparametersandparameter_sensitivitiesstreamlines the input processing and maintains consistent parameter handling across different tile classes. This refactoring improves code modularity and readability.splinepy/microstructure/tiles/cross_3d_linear.py (5)
31-37: Consider adding docstrings for the new class attributesThe new class attributes
_sensitivities_implemented,_closure_directions,_parameter_bounds,_parameters_shape, and_default_parameter_valuehave been added. Including docstrings for these attributes would enhance code readability and help other developers understand their purpose and usage.
39-41: Good job extracting parameter bounds as class constantsExtracting
_BOUNDARY_WIDTH_BOUNDS,_FILLING_HEIGHT_BOUNDS, and_CENTER_EXPANSION_BOUNDSas class constants improves code maintainability and makes it easier to manage and update parameter constraints.
82-85: Refactored input processing enhances code maintainabilityBy utilizing
_process_inputto handleparametersandparameter_sensitivities, the code reduces redundancy and centralizes input validation, which improves maintainability and readability.
87-92: Parameter validation is well-structuredThe use of
_check_custom_parameteralong with class constants for bounds in_closing_tilemethod streamlines parameter validation and ensures consistency across the codebase.
473-475: Parameter validation is well-structuredThe refactoring in the
create_tilemethod enhances code maintainability by centralizing parameter validation using_check_custom_parameterand class constants for bounds.splinepy/microstructure/tiles/cross_2d.py (3)
85-90: Validation of Custom Parameters is Correct and ConsistentThe use of
_check_custom_parameterto validateboundary_widthandfilling_heightagainst their respective bounds is appropriate and ensures that the custom parameters are within valid ranges.
80-83: Consistent Use of_process_inputfor Parameter HandlingThe refactoring to use
_process_inputfor handlingparametersandparameter_sensitivitiesin both_closing_tileandcreate_tilemethods enhances code clarity and reduces redundancy.Also applies to: 413-416
37-39: Proper Definition of Parameter Bound ConstantsThe constants
_BOUNDARY_WIDTH_BOUNDS,_FILLING_HEIGHT_BOUNDS, and_CENTER_EXPANSION_BOUNDSare appropriately defined, providing clear boundaries for parameter validation.splinepy/microstructure/tiles/tile_base.py (2)
303-315: Handle default value types that are neitherfloatnornp.ndarray.In the
_process_inputmethod, whenparametersisNone, you check ifdefault_valueis an instance offloatornp.ndarray. If it's neither, the code does not handle this case, which could lead to unexpected errors.Consider adding an
elseclause to handle unsupporteddefault_valuetypes or raise an appropriate error to inform users about acceptable types.
220-223: Ensure proper broadcasting when checking parameter bounds.In
check_params, when verifying if parameters are within bounds, you're comparingparameters.ravel()withlower_boundsandupper_bounds. Ensure that the shapes of these arrays are compatible for element-wise comparison to avoid broadcasting issues.Please confirm that
parameters.ravel()andlower_boundshave compatible shapes or adjust the comparison to correctly handle differing shapes.splinepy/microstructure/tiles/hollow_octagon_extrude.py (1)
61-66: Ensure consistent parameter checking usingTileBasemethods.In
create_tile, you use_check_custom_parameterto validatecontact_length. Ensure that all custom parameters, includingparameters, are validated consistently using the methods provided byTileBase.splinepy/microstructure/tiles/cross_3d.py (8)
31-37: Class attributes are appropriately defined.The new class attributes
_sensitivities_implemented,_closure_directions,_parameter_bounds,_parameters_shape, and_default_parameter_valueare correctly added and align with the class's requirements.
39-41: Bounds for custom parameters are correctly established.The definitions of
_BOUNDARY_WIDTH_BOUNDS,_FILLING_HEIGHT_BOUNDS, and_CENTER_EXPANSION_BOUNDSprovide clear constraints for parameter validation.
82-85: Consistent parameter processing with_process_input.Using
self._process_inputensures thatparametersandparameter_sensitivitiesare handled consistently, improving code maintainability.
87-92: Effective validation ofboundary_widthandfilling_height.The use of
_check_custom_parameterwith the defined bounds ensures that these parameters stay within valid ranges, enhancing robustness.
517-519: Validation ofcenter_expansionparameter.Checking
center_expansionagainst_CENTER_EXPANSION_BOUNDSvia_check_custom_parameterappropriately enforces parameter constraints.
521-524: Consistent use of_process_inputincreate_tilemethod.This maintains consistency in parameter handling across methods, which is good practice.
528-528: Validation ofparametersagainst calculatedmax_radius.By checking that
parametersdo not exceedmax_radius, the implementation prevents invalid configurations that could lead to geometric inconsistencies.
535-538: Appropriate error handling for unsupported closure directions.Raising
NotImplementedErrorwith a clear message when an unsupportedclosureis provided improves code robustness and user feedback.tests/test_microstructure.py (1)
168-169: Verify the sign convention in derivative comparisonThe assertion compares the negative of the implemented derivative to the finite difference derivative. Please verify that this negative sign is intentional and that the sign convention is consistent.
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
splinepy/microstructure/tiles/cross_2d.py(3 hunks)splinepy/microstructure/tiles/cross_3d.py(3 hunks)splinepy/microstructure/tiles/cross_3d_linear.py(3 hunks)splinepy/microstructure/tiles/snappy.py(6 hunks)splinepy/microstructure/tiles/tile_base.py(5 hunks)tests/test_microstructure.py(1 hunks)tests/test_microtiles.py(1 hunks)
🧰 Additional context used
📓 Learnings (7)
tests/test_microtiles.py (2)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:241-248
Timestamp: 2025-01-20T13:25:54.817Z
Learning: The `generate_random_parameters` function in test_tile_derivatives() is correctly scoped as a local helper function and does not need extraction since it's not duplicated elsewhere in the codebase.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:157-159
Timestamp: 2025-01-16T16:28:22.607Z
Learning: In splinepy's microstructure tests, $C_0$-continuity at closure boundaries is implicitly verified through surface integral tests. If the surface integral equals 1, it indicates no gaps or overlaps exist between patches at the closure boundary.
splinepy/microstructure/tiles/cross_2d.py (2)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:417-419
Timestamp: 2025-01-20T09:43:40.691Z
Learning: In the Cross2D tile class, parameter bounds will be dynamically updated based on the center_expansion value, eliminating the need for separate max_radius validation.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/cross_3d_linear.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
splinepy/microstructure/tiles/cross_3d.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:78-88
Timestamp: 2025-01-17T16:30:34.847Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept within individual tile classes rather than being moved to TileBase, as each tile can have different validation requirements.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
tests/test_microstructure.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microstructure.py:51-52
Timestamp: 2025-01-16T14:31:31.072Z
Learning: When testing closing faces in microstructure tests, boundary ID assertions are not needed as the test only uses specific boundary IDs (2 and 3) as temporary identifiers for min and max boundaries to verify surface areas.
splinepy/microstructure/tiles/tile_base.py (6)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:351-354
Timestamp: 2025-01-20T13:14:48.465Z
Learning: In the splinepy codebase, parameter bounds validation uses open intervals (strict inequalities), and error messages use parentheses notation (a, b) to indicate this. This is evident in the `_check_custom_parameter` method of the `TileBase` class.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-01-20T13:09:40.403Z
Learning: In the TileBase class of splinepy, all property methods except `parameter_bounds` are designed to be class methods (using `cls` parameter) as they access class-level attributes meant to be overridden by subclasses. Only `parameter_bounds` is an instance method as it returns instance-dependent values.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: In splinepy's TileBase, parameter shape validation is handled through two mechanisms:
1. Test-time validation of default_parameter_value dimensions in test_microtiles.py
2. Runtime validation of final parameter shape in TileBase.check_params()
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: Parameter shape validation in splinepy's TileBase is implemented through multiple layers:
1. Class-level validation in test_tile_class ensures _parameters_shape exists and is a tuple
2. Runtime validation in TileBase.check_params() verifies parameter dimensions match (evaluation_points.shape[0], n_info_per_eval_point)
3. Comprehensive testing in test_tile_bounds validates both default and boundary parameter cases
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/snappy.py (2)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:26-29
Timestamp: 2025-01-16T16:38:32.816Z
Learning: In tile classes where parameters are not implemented (like Snappy), empty parameter bounds (`_parameter_bounds = []`) and shape (`_parameters_shape = ()`) are appropriate, as there are no parameters to constrain or shape.
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: build_and_tests (3.12, windows-latest)
- GitHub Check: build_and_tests (3.12, macos-latest)
- GitHub Check: build_and_tests (3.12, ubuntu-20.04)
- GitHub Check: build_and_tests (3.11, windows-latest)
- GitHub Check: build_and_tests (3.11, macos-latest)
- GitHub Check: build_and_tests (3.11, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-latest)
- GitHub Check: build_and_tests (3.10, ubuntu-20.04)
- GitHub Check: build_and_tests (3.9, windows-latest)
- GitHub Check: build_and_tests (3.9, macos-latest)
- GitHub Check: build_and_tests (3.9, ubuntu-20.04)
- GitHub Check: build_and_tests (3.8, windows-latest)
- GitHub Check: build_and_tests (3.8, macos-latest)
- GitHub Check: build_and_tests (3.8, ubuntu-20.04)
🔇 Additional comments (28)
splinepy/microstructure/tiles/snappy.py (5)
26-29: LGTM: Class attributes are correctly defined.The empty parameter bounds and shape are appropriate since parameters aren't implemented yet, as confirmed by the retrieved learning from markriegler.
302-302: LGTM: Return statement moved outside conditional.Moving the return statement outside the conditional block in
_closing_tileensures consistent return type.
356-358: LGTM: Parameter validation is properly handled.The custom parameter validation is correctly implemented using the
_check_custom_parametermethod inherited fromTileBase.
375-375: Fixed typo in error message.The typo in "Parametrization" has been corrected.
11-12: 🧹 Nitpick (assertive)TODO comment is inconsistent with implementation.
The TODO comment states that parameters are not implemented "as the parameters variable", which is unclear and grammatically incorrect.
- # TODO: currently the tile parameters are not implemented as the parameters variable - therefore, no parameter sensitivities are calculated + # TODO: Parameters are not implemented yet, therefore parameter sensitivities + # cannot be calculatedLikely invalid or redundant comment.
tests/test_microtiles.py (1)
1-10: LGTM! Well-organized imports and appropriate epsilon value.The imports are properly organized and the epsilon value (1e-8) is suitable for floating-point comparisons in this context.
tests/test_microstructure.py (3)
27-105: LGTM! Well-structured test implementation.The test function is well-documented, uses proper pytest practices, and includes descriptive error messages. The closure verification logic is robust and the helper function is well-structured.
137-137: 🧹 Nitpick (assertive)Consider using a fixed seed for reproducible tests.
While using random points is good for thorough testing, it might lead to flaky tests. Consider using a fixed seed for reproducibility while maintaining test coverage.
- eval_points = np_rng.random((n_test_points, tile_creator._para_dim)) + # Use fixed seed for reproducible tests + eval_points = np.array([ + [0.1, 0.2], + [0.3, 0.4], + [0.5, 0.6], + [0.7, 0.8], + [0.9, 1.0] + ])[:n_test_points, :tile_creator._para_dim]Likely invalid or redundant comment.
178-181: Verify the sign convention in derivative comparison.The comparison shows a sign mismatch between implemented derivatives and finite differences. Please verify if this is intentional or if there's an inconsistency in the derivative calculations.
✅ Verification successful
The sign convention in the derivative comparison is correct and intentional.
The negative sign is required because the finite difference is calculated as
(original - perturbed)/hinstead of the conventional(perturbed - original)/h, making the implemented derivatives opposite in sign to the finite differences.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other derivative tests to verify the sign convention rg -l "derivative.*finite.*difference" --type pyLength of output: 50
Script:
#!/bin/bash # Search with broader patterns for derivative testing echo "=== Searching for derivative patterns ===" rg "deriv" --type py -l echo -e "\n=== Examining the specific test file ===" cat tests/test_microstructure.pyLength of output: 9038
splinepy/microstructure/tiles/cross_3d_linear.py (8)
39-43: Dynamic computation of_parameter_boundsaddresses inconsistenciesThe
_parameter_boundsproperty now dynamically computes the parameter bounds based on_center_expansion, ensuring consistency and preventing potentialValueErrordue to mismatched bounds. This effectively resolves previous concerns about parameter validation.
45-47: Good practice: Extracted boundary constants improve maintainabilityDefining
_BOUNDARY_WIDTH_BOUNDS,_FILLING_HEIGHT_BOUNDS, and_CENTER_EXPANSION_BOUNDSas class constants enhances code readability and maintainability by centralizing parameter bounds. This makes it easier to manage and update these values in the future.
88-91: Refactored parameter processing improves code clarityUsing
_process_inputto handleparametersandparameter_sensitivitiesin the_closing_tilemethod simplifies the code and enhances maintainability. This refactoring promotes code reuse and reduces redundancy.
93-98: Improved parameter validation using_check_custom_parameterRefactoring the validation of
boundary_widthandfilling_heightusing_check_custom_parameteralong with class constants enhances code consistency and reduces redundancy. This approach ensures that parameter checks are centralized and easily maintainable.
479-481: Consistent parameter validation forcenter_expansionUsing
_check_custom_parameterwith_CENTER_EXPANSION_BOUNDSfor validatingcenter_expansionensures consistent parameter handling across the class. This improves code robustness by centralizing validation logic.
483-483: Assigningcenter_expansionto instance attribute improves encapsulationStoring
center_expansionin the instance attributeself._center_expansionallows each instance ofCross3DLinearto maintain its own state. This enhances object-oriented design principles by encapsulating the attribute within the instance.
485-488: Consistent parameter processing increate_tilemethodUsing
_process_inputin thecreate_tilemethod ensures consistent handling ofparametersandparameter_sensitivitiesthroughout the class. This maintains uniformity and reduces potential errors from inconsistent parameter processing.
31-34: 🧹 Nitpick (assertive)Add docstrings for new class attributes
To enhance code readability and maintainability, consider adding docstrings for the newly added class attributes
_sensitivities_implemented,_closure_directions,_parameters_shape, and_default_parameter_value. This will help other developers understand the purpose and usage of these attributes.⛔ Skipped due to learnings
Learnt from: markriegler PR: tataratat/splinepy#457 File: splinepy/microstructure/tiles/cross_3d.py:31-37 Timestamp: 2025-01-17T16:40:03.329Z Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.Learnt from: markriegler PR: tataratat/splinepy#457 File: splinepy/microstructure/tiles/cross_3d.py:31-37 Timestamp: 2025-01-17T16:40:03.328Z Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.Learnt from: markriegler PR: tataratat/splinepy#457 File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487 Timestamp: 2025-01-20T09:38:11.328Z Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.Learnt from: markriegler PR: tataratat/splinepy#457 File: splinepy/microstructure/tiles/cross_3d.py:513-528 Timestamp: 2025-01-17T16:31:27.419Z Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept specific to each tile implementation rather than consolidated in TileBase, as each tile has unique requirements and constraints.Learnt from: markriegler PR: tataratat/splinepy#457 File: splinepy/microstructure/tiles/cross_3d.py:78-88 Timestamp: 2025-01-17T16:30:34.847Z Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept within individual tile classes rather than being moved to TileBase, as each tile can have different validation requirements.splinepy/microstructure/tiles/cross_2d.py (1)
38-42: Dynamic_parameter_boundsProperty Implemented CorrectlyThe
_parameter_boundsproperty now dynamically computes bounds based oncenter_expansion, effectively addressing previous concerns regarding parameter validation inconsistencies.splinepy/microstructure/tiles/tile_base.py (4)
37-41: Initialization of class attributes is appropriate.The class attributes
_sensitivities_implemented,_closure_directions,_parameter_bounds,_parameters_shape, and_default_parameter_valueare correctly initialized toNone. This allows subclasses to override them with specific values as needed.
218-231: Parameter bounds checking incheck_paramsis well implemented.The implementation correctly checks if all parameters are within the specified bounds by comparing them against
lower_boundsandupper_bounds. This ensures that invalid parameter values are caught early, maintaining the integrity of the tile configurations.
286-332: Implementation of_process_inputmethod is robust.The
_process_inputmethod effectively handles default parameters, initializes derivatives, and performs necessary validation checks. Setting default values when parameters areNoneand validating withcheck_paramsandcheck_param_derivativesensures reliable processing of input data.
351-353: 🧹 Nitpick (assertive)Enhance type checking for parameter values in
_check_custom_parameter.Currently, the method checks if
valueis an instance ofintorfloat. If you intend to accept any real numerical value (including NumPy floating types), consider usingnumbers.Realfor a more comprehensive type check.Apply this diff to improve type checking:
+ import numbers # Add at the top of the file if not isinstance(value, (int, float)): raise ValueError(f"Invalid type for {param_name}")Change to:
if not isinstance(value, (int, float)): + if not isinstance(value, numbers.Real): raise ValueError(f"Invalid type for {param_name}")Likely invalid or redundant comment.
splinepy/microstructure/tiles/cross_3d.py (6)
31-34: Well-defined class attributes enhance claritySetting
_sensitivities_implemented,_closure_directions,_parameters_shape, and_default_parameter_valueprovides clear specifications for theCross3Dclass, improving maintainability and readability.
37-38: Initialization of_center_expansionis appropriateDefining the default value of
_center_expansionenhances class clarity and ensures consistent behavior unless overridden.
39-44: Dynamic computation of parameter bounds is correctly implementedThe property
_parameter_boundsappropriately calculatesmax_radiusbased onself._center_expansion, ensuring parameter bounds adjust dynamically as expected.
45-47: Defined parameter bounds improve validation consistencyDefining
_BOUNDARY_WIDTH_BOUNDS,_FILLING_HEIGHT_BOUNDS, and_CENTER_EXPANSION_BOUNDSenhances parameter validation and ensures consistent constraints across methods.
88-91: Consistent parameter processing with_process_inputUsing
_process_inputto handleparametersandparameter_sensitivitiesin both_closing_tileandcreate_tilemethods promotes code reuse and reduces duplication.Also applies to: 529-532
93-98: Proper validation of input parametersThe use of
_check_custom_parameterto validateboundary_width,filling_height, andcenter_expansionensures input parameters are within expected bounds, improving reliability.Also applies to: 523-525
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
splinepy/microstructure/tiles/cross_3d.py (1)
473-475: Improve error message consistency for unsupported closures.The error message in
_closing_tilefor an unsupportedclosurecould be more informative and consistent withcreate_tile. Including the supported closure directions helps users correct their inputs.You can update the error message as follows:
raise NotImplementedError( - "Requested closing dimension is not supported" + f"Closure '{closure}' not implemented. Supported closures are: " + + f"{self._closure_directions}" )splinepy/microstructure/tiles/tile_base.py (1)
246-264: Clarify the return value ofcheck_param_derivatives.The method returns
FalsewhenderivativesisNone, butTrueotherwise. To improve clarity:
- Update the docstring to reflect that the method returns
Trueif derivatives are valid andFalseifderivativesisNone.- Alternatively, consider returning
Truein both cases ifNoneis an acceptable value, or raise an exception ifderivativesbeingNoneis invalid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
splinepy/microstructure/tiles/cross_3d.py(3 hunks)splinepy/microstructure/tiles/snappy.py(5 hunks)splinepy/microstructure/tiles/tile_base.py(5 hunks)tests/conftest.py(2 hunks)tests/test_microstructure.py(1 hunks)tests/test_microtiles.py(1 hunks)
🧰 Additional context used
📓 Learnings (5)
tests/test_microstructure.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microstructure.py:51-52
Timestamp: 2025-01-16T14:31:31.072Z
Learning: When testing closing faces in microstructure tests, boundary ID assertions are not needed as the test only uses specific boundary IDs (2 and 3) as temporary identifiers for min and max boundaries to verify surface areas.
splinepy/microstructure/tiles/snappy.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:31-31
Timestamp: 2025-02-06T15:42:40.613Z
Learning: The Snappy tile in splinepy/microstructure/tiles/snappy.py is a special case that doesn't use the standard parameter implementation due to its complex parameter handling. The tile is currently not actively used, and its parameters are marked as unimplemented with TODO comments.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:26-29
Timestamp: 2025-01-16T16:38:32.816Z
Learning: In tile classes where parameters are not implemented (like Snappy), empty parameter bounds (`_parameter_bounds = []`) and shape (`_parameters_shape = ()`) are appropriate, as there are no parameters to constrain or shape.
tests/test_microtiles.py (2)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:241-248
Timestamp: 2025-01-20T13:25:54.817Z
Learning: The `generate_random_parameters` function in test_tile_derivatives() is correctly scoped as a local helper function and does not need extraction since it's not duplicated elsewhere in the codebase.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:157-159
Timestamp: 2025-01-16T16:28:22.607Z
Learning: In splinepy's microstructure tests, $C_0$-continuity at closure boundaries is implicitly verified through surface integral tests. If the surface integral equals 1, it indicates no gaps or overlaps exist between patches at the closure boundary.
splinepy/microstructure/tiles/cross_3d.py (4)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:78-88
Timestamp: 2025-01-17T16:30:34.847Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept within individual tile classes rather than being moved to TileBase, as each tile can have different validation requirements.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:86-89
Timestamp: 2025-02-06T12:17:24.389Z
Learning: In the Cross2D tile class, `center_expansion` parameter is only relevant for `create_tile` and should not be modified by `_closing_tile`. The value is set in `create_tile` and used for calculating parameter bounds.
splinepy/microstructure/tiles/tile_base.py (6)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:351-354
Timestamp: 2025-01-20T13:14:48.465Z
Learning: In the splinepy codebase, parameter bounds validation uses open intervals (strict inequalities), and error messages use parentheses notation (a, b) to indicate this. This is evident in the `_check_custom_parameter` method of the `TileBase` class.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-01-20T13:09:40.403Z
Learning: In the TileBase class of splinepy, all property methods except `parameter_bounds` are designed to be class methods (using `cls` parameter) as they access class-level attributes meant to be overridden by subclasses. Only `parameter_bounds` is an instance method as it returns instance-dependent values.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: In splinepy's TileBase, parameter shape validation is handled through two mechanisms:
1. Test-time validation of default_parameter_value dimensions in test_microtiles.py
2. Runtime validation of final parameter shape in TileBase.check_params()
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: Parameter shape validation in splinepy's TileBase is implemented through multiple layers:
1. Class-level validation in test_tile_class ensures _parameters_shape exists and is a tuple
2. Runtime validation in TileBase.check_params() verifies parameter dimensions match (evaluation_points.shape[0], n_info_per_eval_point)
3. Comprehensive testing in test_tile_bounds validates both default and boundary parameter cases
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-02-06T16:16:33.758Z
Learning: In the TileBase class, class-level properties are implemented using the `@property` decorator with `cls` parameter, which works for the project's use case despite being less conventional than using a custom `@classproperty` decorator.
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build_and_tests (3.12, windows-latest)
- GitHub Check: build_and_tests (3.12, macos-latest)
- GitHub Check: build_and_tests (3.12, ubuntu-20.04)
- GitHub Check: build_and_tests (3.11, windows-latest)
- GitHub Check: build_and_tests (3.11, macos-latest)
- GitHub Check: build_and_tests (3.11, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-latest)
- GitHub Check: build_and_tests (3.10, ubuntu-20.04)
- GitHub Check: build_and_tests (3.9, windows-latest)
- GitHub Check: build_and_tests (3.9, ubuntu-20.04)
- GitHub Check: build_and_tests (3.8, windows-latest)
- GitHub Check: build_and_tests (3.8, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-14)
- GitHub Check: build_and_tests (3.10, macos-13)
- GitHub Check: build_and_tests (3.10, ubuntu-latest)
🔇 Additional comments (20)
splinepy/microstructure/tiles/cross_3d.py (9)
31-34: Attributes correctly overridden in subclass.The class attributes
_sensitivities_implemented,_closure_directions,_parameters_shape, and_default_parameter_valueare appropriately defined in theCross3Dsubclass, ensuring that the tile behaves as intended according to its specific requirements.
36-37: Default value for_center_expansionis properly initialized.Initializing
_center_expansionto1.0provides a sensible default that maintains the standard size of the center component unless specified otherwise.
39-43: Dynamic computation of_parameter_boundsenhances flexibility.By computing
_parameter_boundsbased on_center_expansion, the code dynamically adjusts the valid range ofparameters, ensuring that they remain within acceptable limits relative to the center expansion factor.
88-91: Refactored input processing improves maintainability.Utilizing
_process_inputin_closing_tilecentralizes the handling ofparametersandparameter_sensitivities, reducing code duplication and enhancing maintainability.
93-98: Parameter validation is correctly implemented.Using
_check_custom_parameterto validateboundary_widthandfilling_heightagainst defined bounds ensures that these parameters are within the expected ranges, preventing potential runtime errors.
523-525: Validatingcenter_expansionstrengthens input safety.Checking
center_expansionagainst_CENTER_EXPANSION_BOUNDSbefore use ensures that the provided value is within the valid range, enhancing the robustness of thecreate_tilemethod.
527-527: Proper assignment of_center_expansion.Assigning
center_expansiontoself._center_expansionretains the value for use in other methods, such as computing_parameter_bounds.
529-532: Consistent input processing with_process_input.Reusing
_process_inputincreate_tileensures consistent processing and validation ofparametersandparameter_sensitivitiesacross methods.
535-539: Comprehensive error message for unsupported closures.The
create_tilemethod provides a clear error message when an unsupportedclosureis specified, listing the supported options, which aids in user debugging.splinepy/microstructure/tiles/tile_base.py (4)
9-31: Enhanced documentation for class attributes.The added docstrings provide clear and detailed descriptions of the class attributes, improving code readability and maintainability.
37-41: Initialization of class-level attributes for subclass overriding.Initializing the class-level attributes to
Nonesets a clear expectation for subclasses to override these values with specific implementations.
145-161: Appropriate use of instance method forparameter_bounds.Using
selfinstead ofclsin theparameter_boundsproperty is appropriate since the parameter bounds can vary between instances, especially for tiles likeCross2D.
352-354: Correctly allowing integer types for parameter values.The updated type check appropriately accepts both
intandfloattypes forvalue, ensuring that integer parameter values are handled correctly.tests/test_microstructure.py (1)
182-184: Verify the use of negative sign in derivative comparisonIn the assertion comparing the implemented derivative and the finite difference derivative, the implemented derivative is negated. Please verify that the negative sign is appropriate and consistent with the expected mathematical formulation.
tests/test_microtiles.py (1)
1-460: Tests are comprehensive and well-structuredThe test functions thoroughly validate the tile classes and their functionalities.
tests/conftest.py (1)
59-65: Add comment about performance considerationsConsider adding a comment explaining the trade-off between test coverage and execution time in choosing the number of test points.
splinepy/microstructure/tiles/snappy.py (4)
26-29: Setting empty parameter bounds and shapes is appropriate for parameterless tilesAssigning
_parameter_bounds = []and_parameters_shape = ()correctly reflects that theSnappytile does not have parameters implemented. This is consistent with the tile's unique handling of parameters.
373-373: Fix typo in error messageThe typo in the error message has been corrected from "Parametriazation" to "Parametrization," enhancing clarity and professionalism in the codebase.
300-301: Ensure the method_closing_tilealways returns a valueAdding the
return (spline_list, None)outside the conditional blocks ensures that the method_closing_tilealways returns the expected values. This prevents potentialNoneTypeerrors whenclosureis not'y_min'or'y_max'.
31-31:⚠️ Potential issueEnsure contact length bounds are consistent with parameter constraints
The upper bound of
_CONTACT_LENGTH_BOUNDSis set to0.5, but according to the constraint(r + contact_length) < 0.5, and with the defaultr = 0.15, the maximum validcontact_lengthshould be0.35to satisfy0.15 + contact_length < 0.5. This discrepancy could lead to runtime errors if acontact_lengthbetween0.35and0.5is provided.Apply this diff to adjust the upper bound:
-_CONTACT_LENGTH_BOUNDS = [0.0, 0.5] +_CONTACT_LENGTH_BOUNDS = [0.0, 0.35]Alternatively, consider updating the parameter check to dynamically enforce the upper bound based on the current value of
r:def create_tile( self, parameters=None, parameter_sensitivities=None, # TODO contact_length=0.1, a=0.1, b=0.2, c=0.3, r=0.15, closure=None, **kwargs, # noqa ARG002 ): # Existing type and value checks for param in [a, b, c, r, contact_length]: if not isinstance(param, float): raise TypeError(f"Invalid Type, {param} is not float") if param < 0: raise ValueError("Invalid parameter, must be > 0.") + # Dynamically adjust the upper bound based on the value of r + max_contact_length = 0.5 - r + self._check_custom_parameter( + contact_length, + "contact length", + [self._CONTACT_LENGTH_BOUNDS[0], max_contact_length], + ) # Rest of the code...This approach ensures that the constraint
(r + contact_length) < 0.5is always satisfied, regardless of the value ofr.Likely invalid or redundant comment.
|
The |
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
splinepy/microstructure/tiles/snappy.py(5 hunks)splinepy/microstructure/tiles/tile_base.py(5 hunks)tests/conftest.py(2 hunks)tests/test_microtiles.py(1 hunks)
🧰 Additional context used
📓 Learnings (3)
tests/test_microtiles.py (2)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:241-248
Timestamp: 2025-01-20T13:25:54.817Z
Learning: The `generate_random_parameters` function in test_tile_derivatives() is correctly scoped as a local helper function and does not need extraction since it's not duplicated elsewhere in the codebase.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:157-159
Timestamp: 2025-01-16T16:28:22.607Z
Learning: In splinepy's microstructure tests, $C_0$-continuity at closure boundaries is implicitly verified through surface integral tests. If the surface integral equals 1, it indicates no gaps or overlaps exist between patches at the closure boundary.
splinepy/microstructure/tiles/snappy.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:31-31
Timestamp: 2025-02-06T15:42:40.613Z
Learning: The Snappy tile in splinepy/microstructure/tiles/snappy.py is a special case that doesn't use the standard parameter implementation due to its complex parameter handling. The tile is currently not actively used, and its parameters are marked as unimplemented with TODO comments.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:26-29
Timestamp: 2025-01-16T16:38:32.816Z
Learning: In tile classes where parameters are not implemented (like Snappy), empty parameter bounds (`_parameter_bounds = []`) and shape (`_parameters_shape = ()`) are appropriate, as there are no parameters to constrain or shape.
splinepy/microstructure/tiles/tile_base.py (8)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:308-310
Timestamp: 2025-02-07T11:12:29.832Z
Learning: The `_logd` method is defined in the `SplinepyBase` class, which is the parent class of `TileBase`. This ensures unified logging behavior across all classes inheriting from `SplinepyBase`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:351-354
Timestamp: 2025-01-20T13:14:48.465Z
Learning: In the splinepy codebase, parameter bounds validation uses open intervals (strict inequalities), and error messages use parentheses notation (a, b) to indicate this. This is evident in the `_check_custom_parameter` method of the `TileBase` class.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-01-20T13:09:40.403Z
Learning: In the TileBase class of splinepy, all property methods except `parameter_bounds` are designed to be class methods (using `cls` parameter) as they access class-level attributes meant to be overridden by subclasses. Only `parameter_bounds` is an instance method as it returns instance-dependent values.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: In splinepy's TileBase, parameter shape validation is handled through two mechanisms:
1. Test-time validation of default_parameter_value dimensions in test_microtiles.py
2. Runtime validation of final parameter shape in TileBase.check_params()
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: Parameter shape validation in splinepy's TileBase is implemented through multiple layers:
1. Class-level validation in test_tile_class ensures _parameters_shape exists and is a tuple
2. Runtime validation in TileBase.check_params() verifies parameter dimensions match (evaluation_points.shape[0], n_info_per_eval_point)
3. Comprehensive testing in test_tile_bounds validates both default and boundary parameter cases
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-02-06T16:16:33.758Z
Learning: In the TileBase class, class-level properties are implemented using the `@property` decorator with `cls` parameter, which works for the project's use case despite being less conventional than using a custom `@classproperty` decorator.
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: build_and_tests (3.12, windows-latest)
- GitHub Check: build_and_tests (3.12, macos-latest)
- GitHub Check: build_and_tests (3.12, ubuntu-20.04)
- GitHub Check: build_and_tests (3.11, windows-latest)
- GitHub Check: build_and_tests (3.11, macos-latest)
- GitHub Check: build_and_tests (3.11, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-latest)
- GitHub Check: build_and_tests (3.10, ubuntu-20.04)
- GitHub Check: build_and_tests (3.9, windows-latest)
- GitHub Check: build_and_tests (3.9, ubuntu-20.04)
- GitHub Check: build_and_tests (3.8, windows-latest)
- GitHub Check: build_and_tests (3.8, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-13)
🔇 Additional comments (23)
splinepy/microstructure/tiles/snappy.py (7)
26-29: LGTM!The empty parameter bounds and shape are appropriate since parameters are not implemented for this tile.
300-300: LGTM!Moving the return statement outside the closure block ensures consistent return type.
349-352: LGTM!Accepting both integers and floats improves usability.
356-358: LGTM!Using _check_custom_parameter for validation is consistent with other tile implementations.
375-375: LGTM!Fixed typo in error message from "Parametriazation" to "Parametrization".
31-31: Verify contact length bounds.The upper bound of 0.5 might be too high as it could violate the constraint
(r + contact_length) < 0.5on line 361.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify contact length bounds consistency # Search for contact length validation across tile implementations rg "contact_length.*<.*0\.5" "splinepy/microstructure/tiles/"Length of output: 146
Response: Contact Length Bounds Verified
The runtime check on line 361 (i.e. verifying that (r + contact_length) < 0.5) confirms that even though the global constant upper bound is set to 0.5, this bound is never blindly applied—it’s always validated in the context of the current r value. In other words, for nonzero r the effective limit on contact_length is automatically reduced. There’s no evidence that the upper bound of 0.5 creates a violation of the (r + contact_length) < 0.5 constraint.
11-12: 🧹 Nitpick (assertive)Improve TODO comment clarity.
The TODO comment should be more specific about why parameters aren't implemented and what's needed to implement them.
- # TODO: currently the tile parameters are not implemented as the parameters variable - therefore, no parameter sensitivities are calculated + # TODO: Parameter implementation pending due to complex parameter handling. + # This tile requires special parameter handling that differs from other tiles. + # See issue #458 for details on parameter sensitivity calculations.Likely invalid or redundant comment.
splinepy/microstructure/tiles/tile_base.py (11)
9-29: Good job documenting the newly introduced attributes.
They align well with the class’s design, and the explanations are clear and consistent.
37-41: New base attributes appear correctly introduced.
They match the retrieved learnings, and setting them to None provides a clean blueprint for subclasses to override.
75-88: Class-level property for n_info_per_eval_point is acceptable.
This usage is consistent with the convention established in this codebase.
117-130: sensitivities_implemented property appears correct.
The logic for raising NotImplementedError in its parent method is consistent with the rest of the code.
131-144: closure_directions property is appropriately added.
Returning the class-level list directly is consistent with other class properties in this file.
145-161: Instance-level property for parameter_bounds is logically sound.
Retrieving bounds on a per-instance basis makes sense, reflecting potential dynamic behavior.
162-175: parameters_shape class-level property is fine.
It aligns with how other attributes are exposed via class-level properties.
176-189: default_parameter_value property looks good.
It follows the same pattern seen in other class-level properties and fits well into the microtile workflow.
288-335: _process_input method is well-structured.
Default parameter handling and subsequent validation steps are clearly organized and maintain readability.
336-361: _check_custom_parameter faithfully enforces open-interval bounds.
This matches the retrieved learnings that underscore the use of open intervals and clear error messages.
219-233: Potential shape mismatch in parameter bounds check.
When performing the comparison "(parameters.ravel() > lower_bounds) & (parameters.ravel() < upper_bounds)", ensure that lower_bounds and upper_bounds have the correct length (matching parameters.ravel().size). If they only match the second dimension (n_info_per_eval_point), NumPy broadcasting will yield a (total_parameters × n_info_per_eval_point) shape, causing incorrect checks and reshaping errors.As a follow-up, run the script below to confirm that "self._parameter_bounds" is defined in a way that matches all parameter entries and doesn't rely on unintended broadcasting:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify consistent usage of _parameter_bounds across the codebase # Look for any assignments or references to _parameter_bounds to confirm shapes rg "_parameter_bounds" -A 5 -B 5 # Also check for any references to parameter_bounds property usage rg "parameter_bounds" -A 5 -B 5Length of output: 42595
Verification of Parameter Bounds Consistency: No Shape Issues Detected
The investigation shows that in every tile the _parameter_bounds list is defined to have one [lower, upper] pair per parameter (matching the flattened parameters array produced by _parameters_shape). The tests in test_microtiles further confirm that the default values and error checks (for values too low or high) are based on the correct shapes. Therefore, the code’s reliance on NumPy broadcasting here is intentional and consistent.
tests/test_microtiles.py (3)
1-30: Good organization of global definitions
Your approach in collecting tile classes, setting EPS, and defining skip_tiles is logical and helps keep the tests consistent. Consider adding a short docstring for EPS to explain its purpose and usage across the test suite.
160-248: Closure tests appear consistent
Systematically checking multiple closure directions with both default and non-default parameters—while verifying no unexpected sensitivities—provides thorough coverage. This is especially helpful for tiles that are skipped for known reasons.
394-460: Thorough checks for invalid parameter scenarios
Your coverage of out-of-bounds and shape mismatch conditions is excellent, ensuring that each tile class raises the correct exceptions. Nice work!tests/conftest.py (2)
18-18: Seeded random generator
Using a fixed seed improves reproducibility and keeps test results consistent across environments. Good choice!
75-85: Neat approach to fourth-order finite differences
Storing the step sizes and weights in a fixture facilitates reuse. If you ever need multiple FD schemes, consider parameterizing them similarly to maintain flexibility.
I will look into it when I am back from holiday. But I know that @j042 was already looking into it |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.py(2 hunks)tests/test_microtiles.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/test_microtiles.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:45-63
Timestamp: 2025-02-07T12:48:24.840Z
Learning: In splinepy.microstructure tile tests, EPS (1e-8) is used to create open sets from bounds and is intentionally chosen to be magnitudes smaller than any implemented bounds in the tile classes, ensuring that min_value + EPS never exceeds max_value - EPS.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:241-248
Timestamp: 2025-01-20T13:25:54.817Z
Learning: The `generate_random_parameters` function in test_tile_derivatives() is correctly scoped as a local helper function and does not need extraction since it's not duplicated elsewhere in the codebase.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:157-159
Timestamp: 2025-01-16T16:28:22.607Z
Learning: In splinepy's microstructure tests, $C_0$-continuity at closure boundaries is implicitly verified through surface integral tests. If the surface integral equals 1, it indicates no gaps or overlaps exist between patches at the closure boundary.
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build_and_tests (3.12, windows-latest)
- GitHub Check: build_and_tests (3.12, macos-latest)
- GitHub Check: build_and_tests (3.12, ubuntu-20.04)
- GitHub Check: build_and_tests (3.11, windows-latest)
- GitHub Check: build_and_tests (3.11, macos-latest)
- GitHub Check: build_and_tests (3.11, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-latest)
- GitHub Check: build_and_tests (3.10, ubuntu-20.04)
- GitHub Check: build_and_tests (3.9, windows-latest)
- GitHub Check: build_and_tests (3.9, ubuntu-20.04)
- GitHub Check: build_and_tests (3.8, windows-latest)
- GitHub Check: build_and_tests (3.8, ubuntu-20.04)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.10, macos-14)
- GitHub Check: build_and_tests (3.10, macos-13)
- GitHub Check: build_and_tests (3.10, ubuntu-latest)
🔇 Additional comments (13)
tests/test_microtiles.py (8)
1-10: Good use of constants and dependencies.
All imports and the EPS constant (1e-8) appear appropriate and consistent with retrieved learnings, where EPS is used to create open parameter bounds.
12-31: Well-structured enumeration of tile classes.
Defining lists/dictionaries to track which tile classes support closure, sensitivities, or should be skipped is tidy. The skip_tiles dictionary also cleanly documents the skip reasons.
33-44: Thorough boundary check for control points.
The helper function check_control_points enforces that control points remain within [0 - EPS, 1 + EPS], ensuring valid domain constraints.
46-64: Robust conversion to open bounds.
make_bounds_feasible correctly adjusts bounds by ±EPS, aligning with the user’s approach.
66-128: Comprehensive tile class validation.
test_tile_class systematically checks for required attributes, methods, and signatures (create_tile, closure, sensitivities), ensuring each microtile meets specifications.
129-159: Proper default parameter checks.
Verifying the type and shape of _default_parameter_value, as well as ensuring it lies within bounds, helps catch misconfigurations early.
198-249: Closure tests are consistent.
Verifying that closure directions remain within valid bounds and sensitivities are None is a solid approach. The skip mechanism is also used correctly.
395-461: Robust invalid parameter tests.
This set of checks (out-of-bounds and dimension mismatches) thoroughly ensures tile classes properly raise ValueError or TypeError.tests/conftest.py (5)
16-19: Seeded random generator for reproducibility.
Returning np.random.default_rng(seed=0) ensures deterministic test outcomes, which is highly beneficial for debugging.
44-55: Clear documentation for heps fixture.
Using 1e-5 with an explanatory docstring helps maintain a good balance between accuracy and avoidance of floating-point round-off in finite difference calculations.
57-66: Sensible number of test points.
n_test_points=10 is a reasonable compromise between coverage and runtime. The docstring clarifies the trade-off well.
68-76: Meaningful big_perturbation fixture.
The choice of 0.1 for out-of-bounds testing is straightforward. Just keep in mind that extremely tight parameter bounds might require skipping or adjusting this value, as mentioned in the docstring.
78-88: Concise finite difference scheme.
Storing stepsizes and weights in fd_derivative_stepsizes_and_weights is an elegant way to implement a fourth-order centered difference.
|
@markriegler is this complete? |
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
splinepy/microstructure/tiles/hollow_octagon_extrude.py (1)
260-264:⚠️ Potential issueFix inconsistent docstring for closure parameter.
The docstring incorrectly describes
closureas an integer parameter with numeric values, but the implementation expects string values like"x_min","x_max", etc.Apply this diff to fix the documentation:
- closure : int - parametric dimension that needs to be closed. Positive values mean - that minimum parametric dimension is requested. That means, - i.e. -2 closes the tile at maximum z-coordinate. - (must currently be either -2 or 2) + closure : str + parametric dimension that needs to be closed, given in the form + "x_min", "x_max", "y_min", or "y_max"tests/test_microstructure.py (1)
195-195: 🧹 Nitpick (assertive)Fix file termination.
There appears to be an incomplete line at the end of the file that should be removed.
Apply this diff to fix the file termination:
- ) - + )
♻️ Duplicate comments (3)
splinepy/microstructure/tiles/cube_void.py (1)
68-68: The rotation matrix derivative correction is appropriate.Based on the retrieved learning, this correction from
[0, 1, 0]to[0, 0, 0]in the middle row is mathematically correct since it represents the derivative of constants with respect to the rotation angle.tests/test_microstructure.py (1)
125-125: 🧹 Nitpick (assertive)Fix typo in docstring.
- Number of testing points int the parametric domain. Defined in conftest.py + Number of testing points in the parametric domain. Defined in conftest.pysplinepy/microstructure/tiles/tile_base.py (1)
349-352:⚠️ Potential issueReplace
assertstatements with proper exception handling.Using
assertfor input validation is not recommended as assertions can be disabled with Python's-Oflag.Apply this diff:
- if not isinstance(bounds, list): - raise TypeError("bounds has to be a list") - if len(bounds) != 2: - raise ValueError("Bounds must consist of a min. and a max. value") + if not isinstance(bounds, list): + raise TypeError("Bounds has to be a list") + if len(bounds) != 2: + raise ValueError("Bounds must consist of a min. and a max. value")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
examples/show_microstructures.py(2 hunks)splinepy/microstructure/microstructure.py(5 hunks)splinepy/microstructure/tiles/__init__.py(2 hunks)splinepy/microstructure/tiles/chi.py(3 hunks)splinepy/microstructure/tiles/cross_2d.py(3 hunks)splinepy/microstructure/tiles/cross_3d.py(3 hunks)splinepy/microstructure/tiles/cross_3d_linear.py(3 hunks)splinepy/microstructure/tiles/cube_void.py(3 hunks)splinepy/microstructure/tiles/double_lattice.py(2 hunks)splinepy/microstructure/tiles/ellips_v_oid.py(4 hunks)splinepy/microstructure/tiles/hollow_cube.py(2 hunks)splinepy/microstructure/tiles/hollow_octagon.py(5 hunks)splinepy/microstructure/tiles/hollow_octagon_extrude.py(3 hunks)splinepy/microstructure/tiles/snappy.py(5 hunks)splinepy/microstructure/tiles/tile_base.py(5 hunks)tests/conftest.py(2 hunks)tests/test_knot_vectors.py(1 hunks)tests/test_microstructure.py(1 hunks)tests/test_microtiles.py(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
splinepy/microstructure/tiles/cube_void.py (2)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cube_void.py:67-67
Timestamp: 2025-01-16T16:08:24.359Z
Learning: In the y-axis rotation matrix derivative, the middle row is [0, 0, 0] because it's the derivative of [0, 1, 0] with respect to the rotation angle. This follows from the fact that y-coordinates remain unchanged during rotation around y-axis, and the derivative of constants is zero.
tests/test_microstructure.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microstructure.py:51-52
Timestamp: 2025-01-16T14:31:31.072Z
Learning: When testing closing faces in microstructure tests, boundary ID assertions are not needed as the test only uses specific boundary IDs (2 and 3) as temporary identifiers for min and max boundaries to verify surface areas.
splinepy/microstructure/tiles/double_lattice.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/hollow_cube.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/cross_3d_linear.py (4)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:86-89
Timestamp: 2025-02-06T12:17:24.389Z
Learning: In the Cross2D tile class, `center_expansion` parameter is only relevant for `create_tile` and should not be modified by `_closing_tile`. The value is set in `create_tile` and used for calculating parameter bounds.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:417-419
Timestamp: 2025-01-20T09:43:40.691Z
Learning: In the Cross2D tile class, parameter bounds will be dynamically updated based on the center_expansion value, eliminating the need for separate max_radius validation.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/cross_2d.py (4)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:417-419
Timestamp: 2025-01-20T09:43:40.691Z
Learning: In the Cross2D tile class, parameter bounds will be dynamically updated based on the center_expansion value, eliminating the need for separate max_radius validation.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:86-89
Timestamp: 2025-02-06T12:17:24.389Z
Learning: In the Cross2D tile class, `center_expansion` parameter is only relevant for `create_tile` and should not be modified by `_closing_tile`. The value is set in `create_tile` and used for calculating parameter bounds.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/chi.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/chi.py:86-92
Timestamp: 2025-01-16T14:27:59.957Z
Learning: In the Chi class's `create_tile` method, the calculation `dalpha * -r * _np.sin(alpha)` is structured to emphasize that it's the derivative of `r * cos(alpha)` with respect to alpha, where the negative sign placement reflects the chain rule application.
splinepy/microstructure/tiles/hollow_octagon_extrude.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/hollow_octagon_extrude.py:94-98
Timestamp: 2025-01-16T14:27:27.307Z
Learning: Parameter bounds validation for tile parameters has been moved to the TileBase class to centralize the validation logic and reduce code duplication across individual tile classes.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:78-88
Timestamp: 2025-01-17T16:30:34.847Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept within individual tile classes rather than being moved to TileBase, as each tile can have different validation requirements.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:513-528
Timestamp: 2025-01-17T16:31:27.419Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept specific to each tile implementation rather than consolidated in TileBase, as each tile has unique requirements and constraints.
splinepy/microstructure/tiles/cross_3d.py (6)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:78-88
Timestamp: 2025-01-17T16:30:34.847Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept within individual tile classes rather than being moved to TileBase, as each tile can have different validation requirements.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:513-528
Timestamp: 2025-01-17T16:31:27.419Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept specific to each tile implementation rather than consolidated in TileBase, as each tile has unique requirements and constraints.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:86-89
Timestamp: 2025-02-06T12:17:24.389Z
Learning: In the Cross2D tile class, `center_expansion` parameter is only relevant for `create_tile` and should not be modified by `_closing_tile`. The value is set in `create_tile` and used for calculating parameter bounds.
splinepy/microstructure/tiles/snappy.py (4)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:31-31
Timestamp: 2025-02-06T15:42:40.613Z
Learning: The Snappy tile in splinepy/microstructure/tiles/snappy.py is a special case that doesn't use the standard parameter implementation due to its complex parameter handling. The tile is currently not actively used, and its parameters are marked as unimplemented with TODO comments.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:26-29
Timestamp: 2025-01-16T16:38:32.816Z
Learning: In tile classes where parameters are not implemented (like Snappy), empty parameter bounds (`_parameter_bounds = []`) and shape (`_parameters_shape = ()`) are appropriate, as there are no parameters to constrain or shape.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
tests/test_microtiles.py (7)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:157-159
Timestamp: 2025-01-16T16:28:22.607Z
Learning: In splinepy's microstructure tests, $C_0$-continuity at closure boundaries is implicitly verified through surface integral tests. If the surface integral equals 1, it indicates no gaps or overlaps exist between patches at the closure boundary.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:51-102
Timestamp: 2025-01-16T16:34:08.331Z
Learning: Each microtile in splinepy has its own set of optional variables with different types and purposes, making it impractical to implement generic tests for optional attributes. The test_tile_class() function focuses on validating required variables that are common across all microtiles.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:241-248
Timestamp: 2025-01-20T13:25:54.817Z
Learning: The `generate_random_parameters` function in test_tile_derivatives() is correctly scoped as a local helper function and does not need extraction since it's not duplicated elsewhere in the codebase.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:248-346
Timestamp: 2025-02-06T15:21:39.471Z
Learning: For derivative testing in splinepy, a fourth-order accurate finite difference scheme is preferred over second-order schemes as it allows for larger step sizes while maintaining accuracy, which helps mitigate numerical instabilities.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:45-63
Timestamp: 2025-02-07T12:48:24.840Z
Learning: In splinepy.microstructure tile tests, EPS (1e-8) is used to create open sets from bounds and is intentionally chosen to be magnitudes smaller than any implemented bounds in the tile classes, ensuring that min_value + EPS never exceeds max_value - EPS.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:250-392
Timestamp: 2025-02-07T13:05:13.735Z
Learning: For numerical comparisons in tests using np.allclose(), it's acceptable to rely on NumPy's default tolerances (rtol=1e-05, atol=1e-08) without explicitly specifying them.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.884Z
Learning: Parameter shape validation in splinepy's TileBase is implemented through multiple layers:
1. Class-level validation in test_tile_class ensures _parameters_shape exists and is a tuple
2. Runtime validation in TileBase.check_params() verifies parameter dimensions match (evaluation_points.shape[0], n_info_per_eval_point)
3. Comprehensive testing in test_tile_bounds validates both default and boundary parameter cases
splinepy/microstructure/tiles/tile_base.py (8)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-01-20T13:09:40.403Z
Learning: In the TileBase class of splinepy, all property methods except `parameter_bounds` are designed to be class methods (using `cls` parameter) as they access class-level attributes meant to be overridden by subclasses. Only `parameter_bounds` is an instance method as it returns instance-dependent values.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: In splinepy's TileBase, parameter shape validation is handled through two mechanisms:
1. Test-time validation of default_parameter_value dimensions in test_microtiles.py
2. Runtime validation of final parameter shape in TileBase.check_params()
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.884Z
Learning: Parameter shape validation in splinepy's TileBase is implemented through multiple layers:
1. Class-level validation in test_tile_class ensures _parameters_shape exists and is a tuple
2. Runtime validation in TileBase.check_params() verifies parameter dimensions match (evaluation_points.shape[0], n_info_per_eval_point)
3. Comprehensive testing in test_tile_bounds validates both default and boundary parameter cases
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-02-06T16:16:33.758Z
Learning: In the TileBase class, class-level properties are implemented using the `@property` decorator with `cls` parameter, which works for the project's use case despite being less conventional than using a custom `@classproperty` decorator.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:351-354
Timestamp: 2025-01-20T13:14:48.465Z
Learning: In the splinepy codebase, parameter bounds validation uses open intervals (strict inequalities), and error messages use parentheses notation (a, b) to indicate this. This is evident in the `_check_custom_parameter` method of the `TileBase` class.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:308-310
Timestamp: 2025-02-07T11:12:29.832Z
Learning: The `_logd` method is defined in the `SplinepyBase` class, which is the parent class of `TileBase`. This ensures unified logging behavior across all classes inheriting from `SplinepyBase`.
🧬 Code Graph Analysis (11)
splinepy/microstructure/tiles/cube_void.py (1)
splinepy/microstructure/tiles/tile_base.py (1)
_process_input(289-334)
splinepy/microstructure/tiles/double_lattice.py (1)
splinepy/microstructure/tiles/tile_base.py (2)
_check_custom_parameter(336-361)_process_input(289-334)
splinepy/microstructure/tiles/hollow_cube.py (1)
splinepy/microstructure/tiles/tile_base.py (1)
_process_input(289-334)
splinepy/microstructure/tiles/ellips_v_oid.py (1)
splinepy/microstructure/tiles/tile_base.py (1)
_process_input(289-334)
splinepy/microstructure/tiles/cross_3d_linear.py (3)
splinepy/microstructure/tiles/cross_2d.py (1)
_parameter_bounds(39-41)splinepy/microstructure/tiles/cross_3d.py (1)
_parameter_bounds(41-43)splinepy/microstructure/tiles/tile_base.py (2)
_process_input(289-334)_check_custom_parameter(336-361)
examples/show_microstructures.py (4)
splinepy/microstructure/microstructure.py (1)
show(669-706)splinepy/multipatch.py (1)
show(470-471)splinepy/spline.py (1)
show(1678-1694)splinepy/helpme/visualize.py (1)
show(523-609)
splinepy/microstructure/tiles/chi.py (1)
splinepy/microstructure/tiles/tile_base.py (1)
_process_input(289-334)
splinepy/microstructure/tiles/hollow_octagon_extrude.py (2)
splinepy/microstructure/tiles/tile_base.py (3)
create_tile(269-287)_check_custom_parameter(336-361)_process_input(289-334)splinepy/microstructure/tiles/hollow_octagon.py (2)
create_tile(402-565)_closing_tile(29-400)
splinepy/microstructure/tiles/cross_3d.py (3)
splinepy/microstructure/tiles/cross_2d.py (1)
_parameter_bounds(39-41)splinepy/microstructure/tiles/cross_3d_linear.py (1)
_parameter_bounds(41-43)splinepy/microstructure/tiles/tile_base.py (2)
_process_input(289-334)_check_custom_parameter(336-361)
splinepy/microstructure/tiles/snappy.py (4)
splinepy/microstructure/tiles/cross_2d.py (1)
_closing_tile(47-376)splinepy/microstructure/tiles/hollow_octagon.py (1)
_closing_tile(29-400)splinepy/microstructure/tiles/armadillo.py (1)
_closing_tile(38-5008)splinepy/microstructure/tiles/tile_base.py (1)
_check_custom_parameter(336-361)
splinepy/microstructure/tiles/tile_base.py (3)
splinepy/microstructure/tiles/cross_2d.py (1)
_parameter_bounds(39-41)splinepy/microstructure/tiles/cross_3d.py (1)
_parameter_bounds(41-43)splinepy/microstructure/tiles/cross_3d_linear.py (1)
_parameter_bounds(41-43)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_tests (3.11, ubuntu-latest)
- GitHub Check: minimal_explicit_build_and_docs
🔇 Additional comments (46)
tests/conftest.py (4)
18-18: Good addition for test reproducibility.Adding a fixed seed ensures that tests using random numbers are reproducible, which aligns with the PR objective of making the tests less flaky.
46-54: LGTM - Well-documented fixture.The
hepsfixture provides a reasonable step size for finite difference calculations with good documentation explaining the trade-offs.
58-65: LGTM - Good balance for test coverage.The choice of 10 test points with documentation about the performance trade-off is appropriate for thorough testing without excessive runtime.
79-87: LGTM - Solid finite difference implementation.The 4th order accurate centered difference scheme is well-implemented with appropriate coefficients. The return type documentation could be more precise, but the implementation is correct.
examples/show_microstructures.py (1)
349-349: Good correction of parameter name typo.Fixing the misspelled parameter name from "seperator_distance" to "separator_distance" improves code correctness and consistency with the API.
Also applies to: 355-355, 368-368
tests/test_knot_vectors.py (1)
32-32: Good API update for explicit conversion.Using
.numpy()method instead ofnp.array()is more explicit and follows better API design patterns for custom objects that provide numpy array representations.splinepy/microstructure/tiles/__init__.py (1)
11-11: Proper addition of new tile to public interface.Adding
cross_3dto both the imports and__all__list correctly exposes the new Cross3D tile through the package interface, following the established pattern for other tiles.Also applies to: 46-46
splinepy/microstructure/microstructure.py (3)
12-12: LGTM! Docstring improvements enhance clarity.The correction of "construction" in both the class docstring and constructor docstring improves documentation quality.
Also applies to: 21-21
458-463: LGTM! Assertions improve API clarity.Replacing the conditional assignments with assertions makes the interface more explicit and ensures type safety. This is a good improvement over the previous None-defaulting approach.
537-539: LGTM! Style improvement with underscore for unused variable.Using
_instead ofifor unused loop variables is a good practice and improves code readability.splinepy/microstructure/tiles/cube_void.py (2)
28-38: LGTM! Sensitivity support attributes are properly configured.The new class attributes effectively enable parameter sensitivity support:
_sensitivities_implemented = Trueenables sensitivity calculations_parameter_boundsdefines reasonable bounds for expansion and rotation parameters_parameters_shapeand_default_parameter_valueprovide proper defaultsThese align with the broader refactoring to standardize sensitivity support across tile classes.
105-108: LGTM! Parameter handling refactoring improves consistency.The replacement of manual parameter defaulting and validation with the
_process_inputmethod centralizes parameter and sensitivity processing, making the code more maintainable and consistent with other tile classes.splinepy/microstructure/tiles/double_lattice.py (2)
23-28: LGTM! Sensitivity attributes properly configured for DoubleLattice.The new class attributes enable parameter sensitivity support:
_sensitivities_implemented = Trueenables sensitivity calculations_parameter_boundsuses the established mathematical formula for lattice constraints_parameters_shapeand_default_parameter_valueprovide appropriate defaults_CONTACT_LENGTH_BOUNDSproperly defines custom parameter boundsThe mathematical formula for parameter bounds was discussed in past comments, and while documentation would be helpful, the implementation is functionally correct.
59-66: LGTM! Centralized parameter validation improves maintainability.The refactoring replaces manual parameter checks with standardized helper methods:
_check_custom_parametervalidates the contact_length parameter_process_inputhandles parameter defaulting, validation, and derivative initializationThis approach is more maintainable and consistent with the broader tile class refactoring.
splinepy/microstructure/tiles/hollow_cube.py (2)
31-34: LGTM! Sensitivity attributes follow established pattern.The new class attributes properly enable parameter sensitivity support for HollowCube:
_sensitivities_implemented = Trueenables sensitivity calculations_parameter_boundsdefines appropriate bounds for the 7 parameters (all [0.0, 0.5])_parameters_shape = (7, 1)matches the evaluation points structure_default_parameter_value = 0.2provides a reasonable defaultThis implementation is consistent with the broader refactoring across all tile classes.
61-64: LGTM! Parameter processing refactoring enhances consistency.The replacement of manual parameter handling with the
_process_inputmethod standardizes parameter validation, defaulting, and derivative initialization. This follows the same pattern applied across all tile classes in this refactoring.tests/test_microstructure.py (5)
1-27: Well-structured test setup with proper issue tracking.The imports, constants, and test configuration are appropriately organized. Good practice referencing issue #458 in the TODO comment.
29-108: Comprehensive closure testing with appropriate skip logic.The test effectively validates closure functionality across all supported tiles and directions. The surface area verification provides an implicit check for C0-continuity at boundaries. Good use of parametrization and detailed error messages.
1-7: LGTM: Clean and appropriate imports.The import structure is well-organized and includes all necessary modules for comprehensive microstructure testing.
8-26: LGTM: Well-structured test configuration.The constants and tile class filtering are appropriately defined. The CLOSURE_FAILS dictionary with issue documentation provides good traceability for known limitations.
110-194: LGTM: Comprehensive derivative validation test.The macro sensitivities test is well-implemented with proper finite difference validation, shape checks, and detailed error messages including evaluation points for debugging.
splinepy/microstructure/tiles/chi.py (1)
1-138: Correctly implements parameter sensitivity support.The Chi class has been properly updated to support parameter sensitivities following the established pattern. The changes include:
- Addition of required class attributes for sensitivity handling
- Proper use of
_process_inputfor unified parameter processing- Correct implementation of derivative calculations
The negative sign placement in line 70 correctly reflects the derivative calculation as noted in the retrieved learnings.
splinepy/microstructure/tiles/ellips_v_oid.py (1)
1-351: Properly implements parameter sensitivity support with correct derivative fixes.The EllipsVoid class has been successfully updated to support parameter sensitivities:
- Added all required sensitivity-related class attributes
- Correctly fixed the
_rotation_matrix_y_derivmethod by zeroing the middle row- Properly integrated with the unified
_process_inputmethod- Well-documented testing limitations in the class docstring
The implementation follows the established pattern and handles the complex ellipsoid transformations correctly.
tests/test_microtiles.py (4)
1-64: Well-organized test utilities and constants.Good design choices:
- Using a dictionary for skip_tiles with documented reasons
- EPS value appropriately chosen per retrieved learnings
- Clear helper functions for control point validation and bound feasibility
161-196: Thorough bounds testing with comprehensive parameter coverage.The test effectively validates that tile control points remain within the unit cube across all parameter extremes. Good use of cartesian product for exhaustive testing, which is acceptable for current tile implementations per retrieved learnings.
198-249: Comprehensive closure testing across parameter space.Well-designed test that validates closure functionality with both default and extreme parameter values. Appropriate skip logic and clear error messages.
395-461: Excellent error handling validation.The test comprehensively validates that tile classes properly handle invalid inputs:
- Out-of-bounds parameters (both too low and too high)
- Incorrectly shaped parameter arrays
- Proper error messages that indicate the use of TileBase validation methods
Well-designed negative testing.
splinepy/microstructure/tiles/cross_2d.py (3)
29-45: Excellent implementation of dynamic parameter bounds!The dynamic
_parameter_boundsproperty elegantly solves the parameter validation issue by automatically adjusting the valid range based oncenter_expansion. This ensures consistent validation between_process_input()and the tile creation methods.
86-96: Clean refactoring of parameter validation!The use of
_process_inputand_check_custom_parametermethods properly centralizes validation logic while maintaining the tile-specific behavior.
415-424: Correct order of operations for parameter processing!The sequence of validating
center_expansion, setting_center_expansion, and then calling_process_inputensures that the dynamic_parameter_boundsproperty uses the correct value during parameter validation.splinepy/microstructure/tiles/snappy.py (4)
11-31: Appropriate configuration for a tile without standard parameters!The empty parameter bounds and shape correctly reflect that this tile doesn't use the standard parameter implementation, as noted in the TODO comment.
296-300: Improved error handling and return consistency!Using
NotImplementedErrorclearly indicates that only y-direction closures are supported, and the return tuple format is consistent with other tile implementations.
375-375: Good typo fix!
349-358:Details
✅ Verification successful
Type validation improvements look good!
The change to accept both
intandfloatimproves usability, and delegating validation to_check_custom_parametermaintains consistency.However, please verify that
_CONTACT_LENGTH_BOUNDS = [0.0, 0.5]is correct given the constraint on line 361:(r + contact_length) < 0.5. With defaultr = 0.15, the maximum valid contact_length would be less than 0.35.
🏁 Script executed:
#!/bin/bash # Check if there are any tests or documentation that validate the contact_length bounds rg -A 5 -B 5 "contact_length.*0\.[45]" tests/ rg -A 5 -B 5 "Snappy.*contact_length" docs/Length of output: 93
I’d like to inspect the constant definition and surrounding logic to confirm the bounds. Let’s pull the relevant lines:
🏁 Script executed:
#!/bin/bash set -e # Show where CONTACT_LENGTH_BOUNDS is defined and the code around line 361 rg -n "_CONTACT_LENGTH_BOUNDS" splinepy/microstructure/tiles/snappy.py sed -n '340,380p' splinepy/microstructure/tiles/snappy.pyLength of output: 1638
No change needed for
_CONTACT_LENGTH_BOUNDSThe static bound
[0.0, 0.5]defines the absolute maximum contact_length (e.g. whenr = 0), and the subsequent checkif not ((r + contact_length) < 0.5): raise ValueError("Inconsistent parameters, must fulfill : 2*r + contact_length < 0.5")enforces the combined constraint for arbitrary
r.splinepy/microstructure/tiles/cross_3d.py (3)
31-47: Consistent implementation with Cross2D!The dynamic parameter bounds and class attributes follow the same pattern as Cross2D, properly adapted for the 3D case with 6 parameters.
88-98: Clean and consistent validation refactoring!The parameter processing follows the established pattern while maintaining tile-specific validation logic.
523-539: Excellent closure validation with helpful error message!The explicit validation of closure directions with a clear error message listing supported options improves the user experience. The center_expansion handling follows the correct pattern established in Cross2D.
splinepy/microstructure/tiles/cross_3d_linear.py (3)
31-47: LGTM! Well-designed dynamic parameter bounds implementation.The implementation correctly handles the dynamic relationship between
_parameter_boundsand_center_expansion, ensuring that radius parameters remain within valid geometric constraints. The use of a property decorator is appropriate here since bounds need to be recalculated when center expansion changes.
88-98: LGTM! Clean refactoring to use centralized parameter processing.The changes improve code maintainability by:
- Using
_process_inputfor uniform parameter and sensitivity handling- Leveraging
_check_custom_parameterwith class constants for bounds validation- Removing redundant manual validation code
479-488: LGTM! Correct order of operations for parameter validation.The implementation correctly:
- Validates and updates
_center_expansionfirst- Then calls
_process_inputwhich will use the updated dynamic bounds- Delegates parameter bounds validation to the base class
This ensures that the dynamic
_parameter_boundsproperty returns the correct values based on the currentcenter_expansionduring validation.splinepy/microstructure/tiles/hollow_octagon.py (2)
21-27: LGTM! Well-defined class attributes for sensitivity support.The attributes correctly define:
- Support for all four closure directions (x/y min/max)
- Fixed parameter bounds appropriate for the hollow octagon geometry
- Reasonable contact length constraints
437-444: LGTM! Clean parameter validation and processing.The method correctly:
- Validates contact_length against defined bounds
- Uses centralized parameter processing
- Properly delegates closure handling
splinepy/microstructure/tiles/hollow_octagon_extrude.py (2)
21-27: Missing z-direction closure support for 3D tile?The
_closure_directionsonly includes x and y directions, but this is a 3D tile. Is the omission of"z_min"and"z_max"intentional based on the tile's geometry, or should they be added for completeness?
61-79: LGTM! Consistent refactoring pattern.The implementation follows the established pattern with proper parameter validation and delegation to
_closing_tilefor closure handling.splinepy/microstructure/tiles/tile_base.py (2)
289-335: Well-structured parameter processing implementation.The
_process_inputmethod provides a clean, centralized way to handle parameter initialization and validation. The separation of concerns between default value handling, derivative initialization, and validation checks makes the code maintainable and testable.
230-233:⚠️ Potential issueFix ValueError constructor - it expects a single string argument.
The
ValueErrorconstructor expects a single string message, but two separate strings are being passed. This will raise aTypeError.Apply this diff to concatenate the error message:
raise ValueError( - f"The following parameters are out of bounds: {out_of_bounds}. ", - f"Expected bounds: lower: {lower_bounds} and upper: {upper_bounds}", + f"The following parameters are out of bounds: {out_of_bounds}. " + f"Expected bounds: lower: {lower_bounds} and upper: {upper_bounds}" )⛔ Skipped due to learnings
Learnt from: markriegler PR: tataratat/splinepy#457 File: splinepy/microstructure/tiles/cross_3d.py:31-37 Timestamp: 2025-01-17T16:40:03.328Z Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.Learnt from: markriegler PR: tataratat/splinepy#457 File: splinepy/microstructure/tiles/cross_3d.py:31-37 Timestamp: 2025-01-17T16:40:03.329Z Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.Learnt from: markriegler PR: tataratat/splinepy#457 File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487 Timestamp: 2025-01-20T09:38:11.328Z Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
The tests I wanted to implement are complete. Also the code is now more unified, which makes it easier for future implementations. |
…n be either class attribute or instance property
Ok, is that yes? |
|
Yes |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (4)
splinepy/microstructure/tiles/cross_2d.py (1)
367-369: 🧹 Nitpick (assertive)Improve error message for unsupported closure directions
Consider providing more helpful error messages that list the supported closure directions.
-raise NotImplementedError( - "Requested closing dimension is not supported" -) +raise NotImplementedError( + f"Requested closing dimension '{closure}' is not supported. " + f"Supported closures are: {self._closure_directions}" +)splinepy/microstructure/tiles/cross_3d_linear.py (1)
429-431: 🧹 Nitpick (assertive)Consider improving error message consistency
For consistency with
create_tileincross_3d.py, consider providing a more informative error message.-raise NotImplementedError( - "Requested closing dimension is not supported" -) +raise NotImplementedError( + f"Requested closing dimension '{closure}' is not supported. " + f"Supported closures are: {self._closure_directions}" +)splinepy/microstructure/tiles/hollow_octagon_extrude.py (2)
48-48:⚠️ Potential issueFix parameter bounds in docstring to match implementation.
The docstring states the parameter value must be between 0.01 and 0.49, but the
_parameter_boundsattribute defines it as [0.0, 0.5].Apply this diff to fix the inconsistency:
- the value must be between 0.01 and 0.49. + the value must be between 0.0 and 0.5 (exclusive).
260-265:⚠️ Potential issueUpdate outdated closure parameter documentation.
The docstring describes
closureas an integer parameter with values -2 or 2, but the implementation uses string values ("x_min", "x_max", "y_min", "y_max").Apply this diff to fix the documentation:
- closure : int - parametric dimension that needs to be closed. Positive values mean - that minimum parametric dimension is requested. That means, - i.e. -2 closes the tile at maximum z-coordinate. - (must currently be either -2 or 2) + closure : str + parametric dimension that needs to be closed, given in the form + "x_min", "x_max", "y_min", or "y_max".
♻️ Duplicate comments (11)
tests/conftest.py (1)
68-75: Fix the typo in the docstring.There's a spelling error in the docstring that was flagged in previous reviews.
- minimum bound to delibarately make the values out of the bounds.""" + minimum bound to deliberately make the values out of the bounds."""splinepy/microstructure/microstructure.py (1)
430-431: LGTM: Making implicit defaults explicit.Based on the previous discussion and retrieved learnings, this change correctly makes the implicit default values explicit (
knot_span_wise=Trueandmacro_sensitivities=False) without changing behavior. This improves code clarity while maintaining backward compatibility.splinepy/microstructure/tiles/cube_void.py (1)
68-68: LGTM: Rotation matrix derivative correction.Based on the previous discussion and retrieved learnings, this correction to
[0, 0, 0]is mathematically correct since it represents the derivative of the constant middle row[0, 1, 0]with respect to the rotation angle around the y-axis.splinepy/microstructure/tiles/hollow_cube.py (1)
31-34: Consider adding documentation for new class attributestests/test_microtiles.py (3)
122-127: Clean type checking with informative error messages.Good use of
__name__attributes for readable type names in error messages.
218-220: Clear assertion for closure sensitivities.The enhanced error message properly shows the actual value if the assertion fails.
47-48: 🧹 Nitpick (assertive)Fix docstring grammar.
- """Helper function. Bounds are understood as open set of min. and max. values. + """Helper function. Bounds are understood as open sets of min. and max. values.splinepy/microstructure/tiles/snappy.py (2)
349-358: Enhanced parameter validation with better user experience.The relaxed type checking and use of
_check_custom_parameterimprove consistency across the codebase.
375-375: Fixed spelling error.Good catch on the typo correction.
splinepy/microstructure/tiles/hollow_octagon.py (1)
69-83: Verify parameter bounds for sensitivity computations.When computing derivatives, ensure that
v_h_void + parameter_sensitivitiesremains within valid bounds [0, 0.5] to prevent invalid control point generation.#!/bin/bash # Description: Check if parameter bounds validation is enforced in base class for sensitivities # Search for sensitivity bounds checking in TileBase ast-grep --pattern 'class TileBase { $$$ def _process_input($$$) { $$$ } $$$ }' # Also check for any bounds validation in _process_input rg -A 10 "def _process_input" splinepy/microstructure/tiles/tile_base.pysplinepy/microstructure/tiles/cross_3d.py (1)
473-475: 🧹 Nitpick (assertive)Enhance error message for unsupported closure directions
The error message for an unsupported
closurevalue could be more informative. Consider including the list of supported closure directions to aid the user in correcting the input.You can modify the error message as:
-raise NotImplementedError( - "Requested closing dimension is not supported" -) +raise NotImplementedError( + f"Requested closing dimension is not supported. " + f"Supported closures are: {self._closure_directions}" +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
examples/show_microstructures.py(2 hunks)splinepy/microstructure/microstructure.py(5 hunks)splinepy/microstructure/tiles/__init__.py(2 hunks)splinepy/microstructure/tiles/chi.py(3 hunks)splinepy/microstructure/tiles/cross_2d.py(3 hunks)splinepy/microstructure/tiles/cross_3d.py(3 hunks)splinepy/microstructure/tiles/cross_3d_linear.py(3 hunks)splinepy/microstructure/tiles/cube_void.py(3 hunks)splinepy/microstructure/tiles/double_lattice.py(2 hunks)splinepy/microstructure/tiles/ellips_v_oid.py(4 hunks)splinepy/microstructure/tiles/hollow_cube.py(2 hunks)splinepy/microstructure/tiles/hollow_octagon.py(5 hunks)splinepy/microstructure/tiles/hollow_octagon_extrude.py(3 hunks)splinepy/microstructure/tiles/snappy.py(5 hunks)splinepy/microstructure/tiles/tile_base.py(5 hunks)tests/conftest.py(2 hunks)tests/test_knot_vectors.py(1 hunks)tests/test_microstructure.py(1 hunks)tests/test_microtiles.py(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
splinepy/microstructure/tiles/cube_void.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cube_void.py:67-67
Timestamp: 2025-01-16T16:08:24.359Z
Learning: In the y-axis rotation matrix derivative, the middle row is [0, 0, 0] because it's the derivative of [0, 1, 0] with respect to the rotation angle. This follows from the fact that y-coordinates remain unchanged during rotation around y-axis, and the derivative of constants is zero.
tests/test_microstructure.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microstructure.py:51-52
Timestamp: 2025-01-16T14:31:31.072Z
Learning: When testing closing faces in microstructure tests, boundary ID assertions are not needed as the test only uses specific boundary IDs (2 and 3) as temporary identifiers for min and max boundaries to verify surface areas.
splinepy/microstructure/microstructure.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/microstructure.py:430-431
Timestamp: 2025-06-02T12:46:44.927Z
Learning: In splinepy microstructure code, when refactoring method signatures to make implicit defaults explicit (changing from `param=None` with conditional logic to `param=True/False`), this preserves behavior while improving code clarity. The change only affects cases where `None` is explicitly passed, which should be avoided in well-designed APIs.
tests/test_microtiles.py (7)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:157-159
Timestamp: 2025-01-16T16:28:22.607Z
Learning: In splinepy's microstructure tests, $C_0$-continuity at closure boundaries is implicitly verified through surface integral tests. If the surface integral equals 1, it indicates no gaps or overlaps exist between patches at the closure boundary.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:51-102
Timestamp: 2025-01-16T16:34:08.331Z
Learning: Each microtile in splinepy has its own set of optional variables with different types and purposes, making it impractical to implement generic tests for optional attributes. The test_tile_class() function focuses on validating required variables that are common across all microtiles.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:241-248
Timestamp: 2025-01-20T13:25:54.817Z
Learning: The `generate_random_parameters` function in test_tile_derivatives() is correctly scoped as a local helper function and does not need extraction since it's not duplicated elsewhere in the codebase.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:248-346
Timestamp: 2025-02-06T15:21:39.471Z
Learning: For derivative testing in splinepy, a fourth-order accurate finite difference scheme is preferred over second-order schemes as it allows for larger step sizes while maintaining accuracy, which helps mitigate numerical instabilities.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:45-63
Timestamp: 2025-02-07T12:48:24.840Z
Learning: In splinepy.microstructure tile tests, EPS (1e-8) is used to create open sets from bounds and is intentionally chosen to be magnitudes smaller than any implemented bounds in the tile classes, ensuring that min_value + EPS never exceeds max_value - EPS.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: tests/test_microtiles.py:250-392
Timestamp: 2025-02-07T13:05:13.735Z
Learning: For numerical comparisons in tests using np.allclose(), it's acceptable to rely on NumPy's default tolerances (rtol=1e-05, atol=1e-08) without explicitly specifying them.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.884Z
Learning: Parameter shape validation in splinepy's TileBase is implemented through multiple layers:
1. Class-level validation in test_tile_class ensures _parameters_shape exists and is a tuple
2. Runtime validation in TileBase.check_params() verifies parameter dimensions match (evaluation_points.shape[0], n_info_per_eval_point)
3. Comprehensive testing in test_tile_bounds validates both default and boundary parameter cases
splinepy/microstructure/tiles/hollow_cube.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/cross_3d_linear.py (4)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:86-89
Timestamp: 2025-02-06T12:17:24.389Z
Learning: In the Cross2D tile class, `center_expansion` parameter is only relevant for `create_tile` and should not be modified by `_closing_tile`. The value is set in `create_tile` and used for calculating parameter bounds.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:417-419
Timestamp: 2025-01-20T09:43:40.691Z
Learning: In the Cross2D tile class, parameter bounds will be dynamically updated based on the center_expansion value, eliminating the need for separate max_radius validation.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/snappy.py (4)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:31-31
Timestamp: 2025-02-06T15:42:40.613Z
Learning: The Snappy tile in splinepy/microstructure/tiles/snappy.py is a special case that doesn't use the standard parameter implementation due to its complex parameter handling. The tile is currently not actively used, and its parameters are marked as unimplemented with TODO comments.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/snappy.py:26-29
Timestamp: 2025-01-16T16:38:32.816Z
Learning: In tile classes where parameters are not implemented (like Snappy), empty parameter bounds (`_parameter_bounds = []`) and shape (`_parameters_shape = ()`) are appropriate, as there are no parameters to constrain or shape.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/double_lattice.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/cross_3d.py (6)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:78-88
Timestamp: 2025-01-17T16:30:34.847Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept within individual tile classes rather than being moved to TileBase, as each tile can have different validation requirements.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:513-528
Timestamp: 2025-01-17T16:31:27.419Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept specific to each tile implementation rather than consolidated in TileBase, as each tile has unique requirements and constraints.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:86-89
Timestamp: 2025-02-06T12:17:24.389Z
Learning: In the Cross2D tile class, `center_expansion` parameter is only relevant for `create_tile` and should not be modified by `_closing_tile`. The value is set in `create_tile` and used for calculating parameter bounds.
splinepy/microstructure/tiles/cross_2d.py (4)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:417-419
Timestamp: 2025-01-20T09:43:40.691Z
Learning: In the Cross2D tile class, parameter bounds will be dynamically updated based on the center_expansion value, eliminating the need for separate max_radius validation.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_2d.py:86-89
Timestamp: 2025-02-06T12:17:24.389Z
Learning: In the Cross2D tile class, `center_expansion` parameter is only relevant for `create_tile` and should not be modified by `_closing_tile`. The value is set in `create_tile` and used for calculating parameter bounds.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d_linear.py:484-487
Timestamp: 2025-01-20T09:38:11.328Z
Learning: In tile classes, parameter bounds validation is handled by the `_parameter_bounds` class variable and the `_process_input()` method inherited from `TileBase`. Additional dynamic bounds checks may be needed in specific cases, such as when the upper bound depends on other parameters like `center_expansion`.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
splinepy/microstructure/tiles/tile_base.py (9)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.328Z
Learning: The `TileBase` class in splinepy defines base attributes (`_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, `_default_parameter_value`) as `None`, which subclasses must override with specific values. The base class provides property getters that raise errors if these values are not set by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:31-37
Timestamp: 2025-01-17T16:40:03.329Z
Learning: The attributes `_sensitivities_implemented`, `_closure_directions`, `_parameter_bounds`, `_parameters_shape`, and `_default_parameter_value` are defined in the `TileBase` class and can be overridden by subclasses.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:513-528
Timestamp: 2025-01-17T16:31:27.419Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept specific to each tile implementation rather than consolidated in TileBase, as each tile has unique requirements and constraints.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.885Z
Learning: In splinepy's TileBase, parameter shape validation is handled through two mechanisms:
1. Test-time validation of default_parameter_value dimensions in test_microtiles.py
2. Runtime validation of final parameter shape in TileBase.check_params()
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:281-293
Timestamp: 2025-01-17T16:15:44.884Z
Learning: Parameter shape validation in splinepy's TileBase is implemented through multiple layers:
1. Class-level validation in test_tile_class ensures _parameters_shape exists and is a tuple
2. Runtime validation in TileBase.check_params() verifies parameter dimensions match (evaluation_points.shape[0], n_info_per_eval_point)
3. Comprehensive testing in test_tile_bounds validates both default and boundary parameter cases
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-02-06T16:16:33.758Z
Learning: In the TileBase class, class-level properties are implemented using the `@property` decorator with `cls` parameter, which works for the project's use case despite being less conventional than using a custom `@classproperty` decorator.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:75-88
Timestamp: 2025-01-20T13:09:40.403Z
Learning: In the TileBase class of splinepy, all property methods except `parameter_bounds` are designed to be class methods (using `cls` parameter) as they access class-level attributes meant to be overridden by subclasses. Only `parameter_bounds` is an instance method as it returns instance-dependent values.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:351-354
Timestamp: 2025-01-20T13:14:48.465Z
Learning: In the splinepy codebase, parameter bounds validation uses open intervals (strict inequalities), and error messages use parentheses notation (a, b) to indicate this. This is evident in the `_check_custom_parameter` method of the `TileBase` class.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/tile_base.py:308-310
Timestamp: 2025-02-07T11:12:29.832Z
Learning: The `_logd` method is defined in the `SplinepyBase` class, which is the parent class of `TileBase`. This ensures unified logging behavior across all classes inheriting from `SplinepyBase`.
splinepy/microstructure/tiles/chi.py (1)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/chi.py:86-92
Timestamp: 2025-01-16T14:27:59.957Z
Learning: In the Chi class's `create_tile` method, the calculation `dalpha * -r * _np.sin(alpha)` is structured to emphasize that it's the derivative of `r * cos(alpha)` with respect to alpha, where the negative sign placement reflects the chain rule application.
splinepy/microstructure/tiles/hollow_octagon_extrude.py (3)
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/hollow_octagon_extrude.py:94-98
Timestamp: 2025-01-16T14:27:27.307Z
Learning: Parameter bounds validation for tile parameters has been moved to the TileBase class to centralize the validation logic and reduce code duplication across individual tile classes.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:78-88
Timestamp: 2025-01-17T16:30:34.847Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept within individual tile classes rather than being moved to TileBase, as each tile can have different validation requirements.
Learnt from: markriegler
PR: tataratat/splinepy#457
File: splinepy/microstructure/tiles/cross_3d.py:513-528
Timestamp: 2025-01-17T16:31:27.419Z
Learning: In splinepy's microstructure module, validation logic for tile parameters should be kept specific to each tile implementation rather than consolidated in TileBase, as each tile has unique requirements and constraints.
🧬 Code Graph Analysis (11)
splinepy/microstructure/tiles/cube_void.py (1)
splinepy/microstructure/tiles/tile_base.py (1)
_process_input(289-334)
splinepy/microstructure/tiles/hollow_cube.py (1)
splinepy/microstructure/tiles/tile_base.py (1)
_process_input(289-334)
splinepy/microstructure/tiles/cross_3d_linear.py (3)
splinepy/microstructure/tiles/cross_2d.py (1)
_parameter_bounds(39-41)splinepy/microstructure/tiles/cross_3d.py (1)
_parameter_bounds(41-43)splinepy/microstructure/tiles/tile_base.py (2)
_process_input(289-334)_check_custom_parameter(336-361)
splinepy/microstructure/tiles/snappy.py (5)
splinepy/microstructure/tiles/cross_2d.py (1)
_closing_tile(47-376)splinepy/microstructure/tiles/cross_3d.py (1)
_closing_tile(49-482)splinepy/microstructure/tiles/hollow_octagon.py (1)
_closing_tile(29-400)splinepy/microstructure/tiles/armadillo.py (1)
_closing_tile(38-5008)splinepy/microstructure/tiles/tile_base.py (1)
_check_custom_parameter(336-361)
splinepy/microstructure/tiles/double_lattice.py (1)
splinepy/microstructure/tiles/tile_base.py (2)
_check_custom_parameter(336-361)_process_input(289-334)
splinepy/microstructure/tiles/ellips_v_oid.py (1)
splinepy/microstructure/tiles/tile_base.py (1)
_process_input(289-334)
splinepy/microstructure/tiles/cross_3d.py (3)
splinepy/microstructure/tiles/cross_2d.py (1)
_parameter_bounds(39-41)splinepy/microstructure/tiles/cross_3d_linear.py (1)
_parameter_bounds(41-43)splinepy/microstructure/tiles/tile_base.py (2)
_process_input(289-334)_check_custom_parameter(336-361)
splinepy/microstructure/tiles/cross_2d.py (4)
splinepy/microstructure/tiles/cross_3d.py (1)
_parameter_bounds(41-43)splinepy/microstructure/tiles/cross_3d_linear.py (1)
_parameter_bounds(41-43)include/splinepy/splines/helpers/basis_functions.hpp (1)
derivatives(320-320)splinepy/microstructure/tiles/tile_base.py (2)
_process_input(289-334)_check_custom_parameter(336-361)
splinepy/microstructure/tiles/tile_base.py (3)
splinepy/microstructure/tiles/cross_2d.py (1)
_parameter_bounds(39-41)splinepy/microstructure/tiles/cross_3d.py (1)
_parameter_bounds(41-43)splinepy/microstructure/tiles/cross_3d_linear.py (1)
_parameter_bounds(41-43)
splinepy/microstructure/tiles/chi.py (1)
splinepy/microstructure/tiles/tile_base.py (1)
_process_input(289-334)
splinepy/microstructure/tiles/hollow_octagon_extrude.py (2)
splinepy/microstructure/tiles/tile_base.py (3)
create_tile(269-287)_check_custom_parameter(336-361)_process_input(289-334)splinepy/microstructure/tiles/hollow_octagon.py (2)
create_tile(402-565)_closing_tile(29-400)
🪛 Pylint (3.3.7)
splinepy/microstructure/tiles/cube_void.py
[warning] 29-29: TODO: clever parameter bounds and checks if given parametrization would
(W0511)
tests/test_microstructure.py
[warning] 45-45: TODO: right now skip tiles which have faulty closures
(W0511)
[convention] 1-1: Missing module docstring
(C0114)
[error] 1-1: Unable to import 'numpy'
(E0401)
[warning] 13-13: Access to a protected member _closure_directions of a client class
(W0212)
[warning] 40-40: Access to a protected member _closure_directions of a client class
(W0212)
[refactor] 83-83: Consider using a generator instead 'sum(volume(patch) for patch in min_patches.patches)'
(R1728)
[refactor] 84-84: Consider using a generator instead 'sum(volume(patch) for patch in max_patches.patches)'
(R1728)
[warning] 97-97: Access to a protected member _dim of a client class
(W0212)
[warning] 99-99: Access to a protected member _dim of a client class
(W0212)
[warning] 103-103: Access to a protected member _closure_directions of a client class
(W0212)
[refactor] 111-111: Too many local variables (24/15)
(R0914)
[warning] 129-129: Access to a protected member _dim of a client class
(W0212)
[warning] 133-133: Access to a protected member _dim of a client class
(W0212)
[warning] 140-140: Access to a protected member _para_dim of a client class
(W0212)
tests/test_microtiles.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'numpy'
(E0401)
[warning] 17-17: Access to a protected member _closure_directions of a client class
(W0212)
[warning] 23-23: Access to a protected member _sensitivities_implemented of a client class
(W0212)
[warning] 130-130: Access to a protected member _parameters_shape of a client class
(W0212)
[warning] 136-136: Access to a protected member _default_parameter_value of a client class
(W0212)
[warning] 139-139: Access to a protected member _parameters_shape of a client class
(W0212)
[warning] 152-152: Access to a protected member _parameter_bounds of a client class
(W0212)
[warning] 185-185: Access to a protected member _parameter_bounds of a client class
(W0212)
[warning] 191-191: Access to a protected member _parameters_shape of a client class
(W0212)
[warning] 209-209: Access to a protected member _closure_directions of a client class
(W0212)
[warning] 213-213: Access to a protected member _closure_directions of a client class
(W0212)
[warning] 235-235: Access to a protected member _parameter_bounds of a client class
(W0212)
[warning] 239-239: Access to a protected member _closure_directions of a client class
(W0212)
[warning] 243-243: Access to a protected member _parameters_shape of a client class
(W0212)
[refactor] 251-251: Too many local variables (25/15)
(R0914)
[warning] 277-277: Access to a protected member _sensitivities_implemented of a client class
(W0212)
[warning] 282-282: Access to a protected member _parameter_bounds of a client class
(W0212)
[warning] 286-286: Access to a protected member _parameters_shape of a client class
(W0212)
[warning] 293-293: Access to a protected member _closure_directions of a client class
(W0212)
[warning] 294-294: Access to a protected member _closure_directions of a client class
(W0212)
[warning] 297-297: Access to a protected member _evaluation_points of a client class
(W0212)
[warning] 298-298: Access to a protected member _n_info_per_eval_point of a client class
(W0212)
[refactor] 300-300: Too many arguments (6/5)
(R0913)
[refactor] 300-300: Too many positional arguments (6/5)
(R0917)
[warning] 407-407: Access to a protected member _parameter_bounds of a client class
(W0212)
[warning] 413-413: Access to a protected member _parameter_bounds of a client class
(W0212)
[warning] 417-417: Access to a protected member _parameters_shape of a client class
(W0212)
[warning] 432-432: Access to a protected member _parameters_shape of a client class
(W0212)
[warning] 448-448: Access to a protected member _parameters_shape of a client class
(W0212)
splinepy/microstructure/tiles/snappy.py
[refactor] 33-33: Too many arguments (8/5)
(R0913)
[refactor] 33-33: Too many positional arguments (8/5)
(R0917)
[refactor] 33-33: Too many local variables (26/15)
(R0914)
[warning] 35-35: Unused argument 'parameters'
(W0613)
[warning] 34-34: Unused argument 'kwargs'
(W0613)
splinepy/microstructure/tiles/ellips_v_oid.py
[warning] 34-34: TODO: clever parameter bounds and checks if given parametrization would
(W0511)
splinepy/microstructure/tiles/hollow_octagon.py
[refactor] 29-29: Too many local variables (25/15)
(R0914)
[refactor] 29-29: Too many statements (61/50)
(R0915)
[error] 382-382: Possibly using variable 'right' before assignment
(E0606)
[error] 383-383: Possibly using variable 'right_top' before assignment
(E0606)
[error] 384-384: Possibly using variable 'bottom' before assignment
(E0606)
[error] 385-385: Possibly using variable 'bottom_left' before assignment
(E0606)
[error] 386-386: Possibly using variable 'left' before assignment
(E0606)
[error] 387-387: Possibly using variable 'top_left' before assignment
(E0606)
[error] 388-388: Possibly using variable 'top' before assignment
(E0606)
[error] 389-389: Possibly using variable 'bottom_right' before assignment
(E0606)
[refactor] 402-402: Too many local variables (26/15)
(R0914)
splinepy/microstructure/tiles/tile_base.py
[error] 76-76: Method 'n_info_per_eval_point' should have "self" as first argument
(E0213)
[error] 118-118: Method 'sensitivities_implemented' should have "self" as first argument
(E0213)
[error] 132-132: Method 'closure_directions' should have "self" as first argument
(E0213)
[error] 163-163: Method 'parameters_shape' should have "self" as first argument
(E0213)
[error] 177-177: Method 'default_parameter_value' should have "self" as first argument
(E0213)
[refactor] 358-358: Simplify chained comparison between the operands
(R1716)
splinepy/microstructure/tiles/hollow_octagon_extrude.py
[refactor] 29-29: Too many local variables (26/15)
(R0914)
[refactor] 241-241: Too many local variables (25/15)
(R0914)
[refactor] 241-241: Too many statements (59/50)
(R0915)
[error] 1236-1236: Possibly using variable 'right' before assignment
(E0606)
[error] 1237-1237: Possibly using variable 'right_top' before assignment
(E0606)
[error] 1238-1238: Possibly using variable 'bottom' before assignment
(E0606)
[error] 1239-1239: Possibly using variable 'bottom_left' before assignment
(E0606)
[error] 1240-1240: Possibly using variable 'left' before assignment
(E0606)
[error] 1241-1241: Possibly using variable 'top_left' before assignment
(E0606)
[error] 1242-1242: Possibly using variable 'top' before assignment
(E0606)
[error] 1243-1243: Possibly using variable 'bottom_right' before assignment
(E0606)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: build_and_tests (3.13, ubuntu-latest)
- GitHub Check: build_and_tests (3.13, windows-latest)
- GitHub Check: build_and_tests (3.12, windows-latest)
- GitHub Check: build_and_tests (3.12, ubuntu-latest)
- GitHub Check: build_and_tests (3.12, macos-latest)
- GitHub Check: build_and_tests (3.10, windows-latest)
- GitHub Check: build_and_tests (3.11, ubuntu-latest)
- GitHub Check: build_and_tests (3.11, windows-latest)
- GitHub Check: build_and_tests (3.10, ubuntu-latest)
- GitHub Check: build_and_tests (3.9, windows-latest)
- GitHub Check: build_and_tests (3.9, ubuntu-latest)
- GitHub Check: build_and_tests (3.9, macos-latest)
- GitHub Check: linux_wheel
- GitHub Check: windows_wheel
- GitHub Check: macos_wheel
- GitHub Check: macos_wheel-m1
- GitHub Check: build_and_tests (3.12, windows-latest)
- GitHub Check: build_and_tests (3.12, ubuntu-latest)
- GitHub Check: build_and_tests (3.12, macos-13)
- GitHub Check: minimal_explicit_build_and_docs
🔇 Additional comments (36)
tests/conftest.py (4)
18-18: Good improvement for test reliability.Setting a fixed seed ensures deterministic random sequences, which addresses the flaky test issues mentioned in the PR objectives.
44-54: Well-implemented fixture for finite difference calculations.The chosen step size and documentation are appropriate for numerical derivative calculations.
57-65: Good balance between test coverage and performance.The documentation clearly explains the trade-off considerations for the chosen value.
78-87: Mathematically correct finite difference coefficients.The fourth-order centered difference weights are properly implemented.
examples/show_microstructures.py (1)
349-349: Good typo corrections.The spelling corrections from "seperator_distance" to "separator_distance" improve consistency with the parameter naming conventions.
Also applies to: 355-355, 368-368
splinepy/microstructure/tiles/__init__.py (1)
11-11: Proper addition of new tile to public API.The cross_3d module is correctly added to both imports and the all list, following the established pattern.
Also applies to: 46-46
tests/test_knot_vectors.py (1)
32-32: Appropriate API update for knot vector access.The change from
np.array(spl_kv)tospl_kv.numpy()reflects an improved API design for accessing the underlying numpy data.splinepy/microstructure/microstructure.py (2)
12-12: LGTM: Docstring typo fixes.Good correction to improve documentation clarity.
Also applies to: 21-21
458-463: LGTM: Added type validation improves robustness.The assertions ensure parameters are of the expected boolean type, providing better error messages and catching type mismatches early.
splinepy/microstructure/tiles/cube_void.py (2)
28-38: LGTM: Standardized sensitivity support attributes.The new class attributes (
_sensitivities_implemented,_parameter_bounds,_parameters_shape,_default_parameter_value) align with the broader refactoring effort to standardize parameter handling across tile classes. The parameter bounds for expansions and rotations are appropriate for the unit cube context.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 29-29: TODO: clever parameter bounds and checks if given parametrization would
(W0511)
105-108: LGTM: Refactored to use centralized parameter processing.Replacing manual parameter handling with
_process_inputcentralizes the validation and initialization logic, improving consistency and maintainability across tile classes. The method handles default parameter setting, derivatives initialization, and parameter validation as shown in the relevant code snippets.splinepy/microstructure/tiles/double_lattice.py (2)
23-28: LGTM: Standardized sensitivity support attributes.The new class attributes follow the consistent pattern established across tile classes. The parameter bounds formula
1 / (2 * (1 + _np.sqrt(2)))appears to be mathematically derived for the double lattice geometry, and the default parameter value of 0.1 is reasonable.
59-66: LGTM: Refactored parameter validation and processing.The changes replace manual validation and initialization with standardized methods:
_check_custom_parameterfor contact length validation with proper bounds checking_process_inputfor consistent parameter and sensitivity processingThis improves code maintainability and consistency across tile classes.
splinepy/microstructure/tiles/hollow_cube.py (1)
61-64: Well-structured refactoring to centralize parameter processing.The replacement of manual parameter handling with
_process_inputaligns with the broader refactoring pattern across tile classes, improving maintainability and consistency.tests/test_microstructure.py (3)
29-108: Well-structured and comprehensive closure tests.The test function properly handles tiles with and without closure support, includes clear skip logic for known issues, and thoroughly validates boundary surface areas.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 45-45: TODO: right now skip tiles which have faulty closures
(W0511)
[warning] 40-40: Access to a protected member _closure_directions of a client class
(W0212)
[refactor] 83-83: Consider using a generator instead 'sum(volume(patch) for patch in min_patches.patches)'
(R1728)
[refactor] 84-84: Consider using a generator instead 'sum(volume(patch) for patch in max_patches.patches)'
(R1728)
[warning] 97-97: Access to a protected member _dim of a client class
(W0212)
[warning] 99-99: Access to a protected member _dim of a client class
(W0212)
[warning] 103-103: Access to a protected member _closure_directions of a client class
(W0212)
110-194: Thorough macro sensitivity testing with finite differences.The test effectively validates derivative correctness by comparing implemented derivatives against finite difference approximations, with proper shape assertions and detailed error reporting.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 111-111: Too many local variables (24/15)
(R0914)
[warning] 129-129: Access to a protected member _dim of a client class
(W0212)
[warning] 133-133: Access to a protected member _dim of a client class
(W0212)
[warning] 140-140: Access to a protected member _para_dim of a client class
(W0212)
21-21: 🧹 Nitpick (assertive)Add issue reference to TODO comment.
Consider adding the issue reference for better traceability.
Apply this diff:
-# TODO(#458): the following tiles fail the closure test +# TODO(#458): the following tiles fail the closure testLikely an incorrect or invalid review comment.
splinepy/microstructure/tiles/ellips_v_oid.py (2)
98-98: Correct fix for rotation matrix derivative.The middle row of the y-axis rotation derivative matrix should indeed be zeros, as the y-component doesn't change with y-axis rotation.
134-137: Clean refactoring to use centralized parameter processing.The use of
_process_inputremoves code duplication and ensures consistent parameter and sensitivity handling across all tile classes.splinepy/microstructure/tiles/chi.py (2)
19-19: Correct dimension adjustment for evaluation points.The change from 2D array
[[0.5, 0.5]]to 1D array[[0.5]]properly reflects that Chi has_para_dim = 1.
48-51: Consistent implementation of parameter processing and derivative calculation.The refactoring correctly uses
_process_inputfor centralized parameter handling, and the derivative calculations maintain the mathematically correct sign placement for the chain rule application.Also applies to: 64-70
tests/test_microtiles.py (3)
26-31: Well-structured skip configuration!The dictionary format with descriptive skip reasons is a clean approach for test maintenance.
33-44: Robust control point validation with informative error messages.The enhanced error reporting showing specific out-of-bounds points aids debugging.
300-344: Well-implemented fourth-order finite difference scheme.The implementation correctly uses the stepsize-weight pairs for accurate derivative approximation.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 300-300: Too many arguments (6/5)
(R0913)
[refactor] 300-300: Too many positional arguments (6/5)
(R0917)
splinepy/microstructure/tiles/snappy.py (1)
296-301: Improved error handling and return consistency.Using
NotImplementedErrorbetter indicates missing functionality, and the unified return statement improves code structure.splinepy/microstructure/tiles/hollow_octagon.py (1)
437-452: Well-integrated parameter processing and validation.The method properly uses framework utilities for parameter validation and processing.
splinepy/microstructure/tiles/cross_3d.py (2)
31-43: LGTM! Well-structured class attributes and dynamic parameter bounds.The implementation correctly:
- Overrides base class attributes as required
- Implements dynamic parameter bounds that adjust based on
_center_expansion- Follows the same pattern as
Cross2DandCross3DLinear
523-539: Excellent refactoring of parameter validation and closure handling!The changes improve code quality by:
- Properly validating
center_expansionagainst defined bounds- Centralizing parameter processing through
_process_input- Providing clear error messages for unsupported closures
splinepy/microstructure/tiles/cross_2d.py (1)
29-45: Well-implemented sensitivity support and dynamic parameter bounds!The implementation correctly:
- Adds required class attributes for sensitivity support
- Implements dynamic
_parameter_boundsbased on_center_expansion- Defines clear bounds constants for custom parameters
splinepy/microstructure/tiles/cross_3d_linear.py (2)
31-47: Consistent implementation of sensitivity support!The class attributes and dynamic parameter bounds follow the established pattern across all cross tile variants. The bounds constants are appropriately defined.
479-488: Clean refactoring of parameter validation!The implementation properly:
- Validates
center_expansionusing the defined bounds constant- Updates the instance attribute for use in
_parameter_bounds- Centralizes parameter processing through
_process_inputsplinepy/microstructure/tiles/hollow_octagon_extrude.py (3)
21-27: LGTM! Well-structured class attributes for sensitivity and closure support.The new class attributes properly define the tile's capabilities and constraints, following the established pattern from TileBase.
21-27: LGTM! Class attributes properly define tile capabilities.The new class attributes follow the established pattern for tile classes and correctly specify:
- Parameter sensitivity support
- Allowed closure directions for all four boundaries
- Parameter bounds and default values
- Contact length constraints
61-239: Well-structured implementation with proper sensitivity support.The
create_tilemethod has been successfully refactored to:
- Use centralized parameter validation via
_check_custom_parameter- Leverage
_process_inputfor unified parameter handling- Support parameter sensitivities through derivative loops
- Properly delegate closure cases to
_closing_tileThe implementation correctly zeros geometric constants for derivative cases while maintaining the sensitivity parameter contribution.
splinepy/microstructure/tiles/tile_base.py (2)
37-41: Well-designed attribute and property structure for tile configuration.The new attributes and properties provide a clean interface for subclasses to define their capabilities. The documentation clearly explains why
parameter_boundsis instance-dependent while others are class-level.Also applies to: 117-189
289-362: Excellent utility methods for unified parameter handling.The
_process_inputand_check_custom_parametermethods provide clean abstractions that reduce code duplication across tile implementations. The methods properly handle edge cases and provide clear error messages.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 358-358: Simplify chained comparison between the operands
(R1716)
| # TODO: clever parameter bounds and checks if given parametrization would | ||
| # still lie in unit cube | ||
| # Due to ellipsoid, control points very easily lie outside unit cube | ||
| _parameter_bounds = [ | ||
| [0.0, 1.0], | ||
| [0.0, 1.0], | ||
| [-_np.pi / 2, _np.pi / 2], | ||
| [-_np.pi / 2, _np.pi / 2], | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
TODO indicates future parameter bounds enhancement needed.
The comment highlights that control points can lie outside the unit cube. Consider creating an issue to track implementing clever parameter bounds that ensure the tile remains within the unit cube.
Would you like me to help create an issue to track the implementation of parameter bounds validation that ensures the tile remains within the unit cube?
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 34-34: TODO: clever parameter bounds and checks if given parametrization would
(W0511)
🤖 Prompt for AI Agents
In splinepy/microstructure/tiles/ellips_v_oid.py around lines 34 to 42, the TODO
comment notes that current parameter bounds may allow control points outside the
unit cube. To address this, create a new issue in the project tracker describing
the need to implement smarter parameter bounds and validation logic that
guarantees the tile stays within the unit cube. This will help prioritize and
track the enhancement separately from the current code.
|
|
||
| for control_points in [ | ||
| right, | ||
| right_top, | ||
| bottom, | ||
| bottom_left, | ||
| left, | ||
| top_left, | ||
| top, | ||
| bottom_right, | ||
| ]: | ||
| spline_list.append( | ||
| _Bezier(degrees=[1, 1], control_points=control_points) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add error handling for invalid closure directions.
The method should raise an error if the closure direction is not recognized.
else:
- pass
+ raise NotImplementedError(
+ f"Closure direction '{closure}' is not supported. "
+ f"Supported directions are: {self._closure_directions}"
+ )
for control_points in [
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for control_points in [ | |
| right, | |
| right_top, | |
| bottom, | |
| bottom_left, | |
| left, | |
| top_left, | |
| top, | |
| bottom_right, | |
| ]: | |
| spline_list.append( | |
| _Bezier(degrees=[1, 1], control_points=control_points) | |
| ) | |
| else: | |
| raise NotImplementedError( | |
| f"Closure direction '{closure}' is not supported. " | |
| f"Supported directions are: {self._closure_directions}" | |
| ) | |
| for control_points in [ | |
| right, | |
| right_top, | |
| bottom, | |
| bottom_left, | |
| left, | |
| top_left, | |
| top, | |
| bottom_right, | |
| ]: | |
| spline_list.append( | |
| _Bezier(degrees=[1, 1], control_points=control_points) | |
| ) |
🧰 Tools
🪛 Pylint (3.3.7)
[error] 382-382: Possibly using variable 'right' before assignment
(E0606)
[error] 383-383: Possibly using variable 'right_top' before assignment
(E0606)
[error] 384-384: Possibly using variable 'bottom' before assignment
(E0606)
[error] 385-385: Possibly using variable 'bottom_left' before assignment
(E0606)
[error] 386-386: Possibly using variable 'left' before assignment
(E0606)
[error] 387-387: Possibly using variable 'top_left' before assignment
(E0606)
[error] 388-388: Possibly using variable 'top' before assignment
(E0606)
[error] 389-389: Possibly using variable 'bottom_right' before assignment
(E0606)
🤖 Prompt for AI Agents
In splinepy/microstructure/tiles/hollow_octagon.py around lines 380 to 394, the
code currently processes closure directions without validating them. Add error
handling to check if the closure direction is valid before processing; if an
unrecognized closure direction is encountered, raise an appropriate error such
as a ValueError with a clear message indicating the invalid direction.
| v_h_void = parameters[0, 0] | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused variable assignment.
The assignment v_h_void = parameters[0, 0] is not used before the potential delegation to _closing_tile. Consider removing this line to improve code clarity.
- v_h_void = parameters[0, 0]
-
if closure is not None:
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| v_h_void = parameters[0, 0] | |
| if closure is not None: |
🤖 Prompt for AI Agents
In splinepy/microstructure/tiles/hollow_octagon_extrude.py at lines 70 to 71,
the variable v_h_void is assigned from parameters[0, 0] but never used
afterward. Remove the assignment line to eliminate the unused variable and
improve code clarity.
| raise ValueError( | ||
| f"The following parameters are out of bounds: {out_of_bounds}. ", | ||
| f"Expected bounds: lower: {lower_bounds} and upper: {upper_bounds}", | ||
| ) |
There was a problem hiding this comment.
Fix error message concatenation.
The error message uses a comma instead of + operator, which creates a tuple instead of concatenating the strings.
raise ValueError(
- f"The following parameters are out of bounds: {out_of_bounds}. ",
- f"Expected bounds: lower: {lower_bounds} and upper: {upper_bounds}",
+ f"The following parameters are out of bounds: {out_of_bounds}. "
+ f"Expected bounds: lower: {lower_bounds} and upper: {upper_bounds}"
)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise ValueError( | |
| f"The following parameters are out of bounds: {out_of_bounds}. ", | |
| f"Expected bounds: lower: {lower_bounds} and upper: {upper_bounds}", | |
| ) | |
| raise ValueError( | |
| f"The following parameters are out of bounds: {out_of_bounds}. " | |
| f"Expected bounds: lower: {lower_bounds} and upper: {upper_bounds}" | |
| ) |
🤖 Prompt for AI Agents
In splinepy/microstructure/tiles/tile_base.py around lines 230 to 233, the error
message in the ValueError is incorrectly using a comma to separate strings,
which results in a tuple instead of a concatenated string. Replace the comma
with a plus operator or use a single formatted string to properly concatenate
the message parts into one string.
| if closure is None: | ||
| raise ValueError("No closing direction given") | ||
|
|
||
| if parameters is None: | ||
| self._log("Tile request is not parametrized, setting default 0.2") | ||
| parameters = _np.array( | ||
| _np.ones( | ||
| (len(self._evaluation_points), self._n_info_per_eval_point) | ||
| ) | ||
| * 0.2 | ||
| ) | ||
|
|
||
| if not (_np.all(parameters[0] > 0) and _np.all(parameters[0] < 0.5)): | ||
| raise ValueError( | ||
| "The thickness of the wall must be in (0.01 and 0.49)" | ||
| ) | ||
|
|
||
| self.check_params(parameters) | ||
|
|
||
| if parameter_sensitivities is not None: | ||
| raise NotImplementedError( | ||
| "Derivatives are not implemented for this tile yet" | ||
| ) | ||
| # Process input | ||
| parameters, n_derivatives, derivatives = self._process_input( | ||
| parameters=parameters, | ||
| parameter_sensitivities=parameter_sensitivities, | ||
| ) | ||
|
|
||
| v_h_void = parameters[0, 0] | ||
| if not ((v_h_void > 0.01) and (v_h_void < 0.5)): | ||
| raise ValueError( | ||
| "The thickness of the wall must be in (0.01 and 0.49)" | ||
| ) | ||
|
|
||
| spline_list = [] | ||
| v_zero = 0.0 | ||
| v_one_half = 0.5 | ||
| v_one = 1.0 | ||
| v_outer_c_h = contact_length * 0.5 | ||
| v_inner_c_h = contact_length * parameters[0, 0] | ||
|
|
||
| if closure == "x_min": | ||
| # set points: | ||
| right = _np.array( | ||
| [ | ||
| [v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_one, -v_outer_c_h + v_one_half], | ||
| [v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_one, v_outer_c_h + v_one_half], | ||
| ] | ||
| ) | ||
|
|
||
| right_top = _np.array( | ||
| [ | ||
| [v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_one, v_outer_c_h + v_one_half], | ||
| [v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| top = _np.array( | ||
| [ | ||
| [v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_one], | ||
| [-v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| bottom_left = _np.array( | ||
| [ | ||
| [-v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_zero, v_zero], | ||
| [-v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_zero], | ||
| ] | ||
| ) | ||
|
|
||
| left = _np.array( | ||
| [ | ||
| [v_zero, v_zero], | ||
| [-v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_zero, v_one], | ||
| [-v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| ] | ||
| ) | ||
|
|
||
| top_left = _np.array( | ||
| [ | ||
| [v_zero, v_one], | ||
| [-v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_one], | ||
| [-v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| ] | ||
| ) | ||
|
|
||
| bottom = _np.array( | ||
| [ | ||
| [v_outer_c_h + v_one_half, v_zero], | ||
| [v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_zero], | ||
| [-v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| ] | ||
| ) | ||
| splines = [] | ||
| for i_derivative in range(n_derivatives + 1): | ||
| if i_derivative == 0: | ||
| v_zero = 0.0 | ||
| v_one_half = 0.5 | ||
| v_one = 1.0 | ||
| v_outer_c_h = contact_length * 0.5 | ||
| v_inner_c_h = contact_length * parameters[0, 0] | ||
| else: | ||
| v_h_void = parameter_sensitivities[0, 0, i_derivative - 1] | ||
| v_zero, v_one_half, v_one = [0.0, 0.0, 0.0] | ||
| v_outer_c_h = 0.0 | ||
| v_inner_c_h = contact_length * v_h_void | ||
|
|
||
| spline_list = [] | ||
|
|
||
| if closure == "x_min": | ||
| # set points: | ||
| right = _np.array( | ||
| [ | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| bottom_right = _np.array( | ||
| [ | ||
| [v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_zero], | ||
| [v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_one, -v_outer_c_h + v_one_half], | ||
| ] | ||
| ) | ||
| right_top = _np.array( | ||
| [ | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| elif closure == "x_max": | ||
| right = _np.array( | ||
| [ | ||
| [v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_one, v_zero], | ||
| [v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_one, v_one], | ||
| ] | ||
| ) | ||
| top = _np.array( | ||
| [ | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| right_top = _np.array( | ||
| [ | ||
| [v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_one, v_one], | ||
| [v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
| bottom_left = _np.array( | ||
| [ | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_zero, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_zero, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| top = _np.array( | ||
| [ | ||
| [v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_one], | ||
| [-v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
| left = _np.array( | ||
| [ | ||
| [v_zero, v_zero, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_one, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_zero, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_one, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| bottom_left = _np.array( | ||
| [ | ||
| [-v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_zero, -v_outer_c_h + v_one_half], | ||
| [-v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_zero], | ||
| ] | ||
| ) | ||
| top_left = _np.array( | ||
| [ | ||
| [v_zero, v_one, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_one, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| left = _np.array( | ||
| [ | ||
| [v_zero, -v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_zero, v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| ] | ||
| ) | ||
| bottom = _np.array( | ||
| [ | ||
| [v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| top_left = _np.array( | ||
| [ | ||
| [v_zero, v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_one], | ||
| [-v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| ] | ||
| ) | ||
| bottom_right = _np.array( | ||
| [ | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| bottom = _np.array( | ||
| [ | ||
| [v_outer_c_h + v_one_half, v_zero], | ||
| [v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_zero], | ||
| [-v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| ] | ||
| ) | ||
| elif closure == "x_max": | ||
| right = _np.array( | ||
| [ | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_zero, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_one, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_zero, v_one], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| bottom_right = _np.array( | ||
| [ | ||
| [v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_zero], | ||
| [v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_one, v_zero], | ||
| ] | ||
| ) | ||
| right_top = _np.array( | ||
| [ | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_one, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_one, v_one], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| elif closure == "y_min": | ||
| # set points: | ||
| right = _np.array( | ||
| [ | ||
| [v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_one, -v_outer_c_h + v_one_half], | ||
| [v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_one, v_outer_c_h + v_one_half], | ||
| ] | ||
| ) | ||
| top = _np.array( | ||
| [ | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| right_top = _np.array( | ||
| [ | ||
| [v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_one, v_outer_c_h + v_one_half], | ||
| [v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
| bottom_left = _np.array( | ||
| [ | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| top = _np.array( | ||
| [ | ||
| [v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_one], | ||
| [-v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
| left = _np.array( | ||
| [ | ||
| [v_zero, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| bottom_left = _np.array( | ||
| [ | ||
| [-v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_zero, -v_outer_c_h + v_one_half], | ||
| [-v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [v_zero, v_zero], | ||
| ] | ||
| ) | ||
| top_left = _np.array( | ||
| [ | ||
| [v_zero, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| left = _np.array( | ||
| [ | ||
| [v_zero, -v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_zero, v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| ] | ||
| ) | ||
| bottom = _np.array( | ||
| [ | ||
| [v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| top_left = _np.array( | ||
| [ | ||
| [v_zero, v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_one], | ||
| [-v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| ] | ||
| ) | ||
| bottom_right = _np.array( | ||
| [ | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_zero, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_zero, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| bottom = _np.array( | ||
| [ | ||
| [v_one, v_zero], | ||
| [v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [v_zero, v_zero], | ||
| [-v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| ] | ||
| ) | ||
| elif closure == "y_min": | ||
| # set points: | ||
| right = _np.array( | ||
| [ | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| bottom_right = _np.array( | ||
| [ | ||
| [v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [v_one, v_zero], | ||
| [v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_one, -v_outer_c_h + v_one_half], | ||
| ] | ||
| ) | ||
| right_top = _np.array( | ||
| [ | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| elif closure == "y_max": | ||
| # set points: | ||
| right = _np.array( | ||
| [ | ||
| [v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_one, -v_outer_c_h + v_one_half], | ||
| [v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_one, v_outer_c_h + v_one_half], | ||
| ] | ||
| ) | ||
| top = _np.array( | ||
| [ | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_one, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| right_top = _np.array( | ||
| [ | ||
| [v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_one, v_outer_c_h + v_one_half], | ||
| [v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_one, v_one], | ||
| ] | ||
| ) | ||
| bottom_left = _np.array( | ||
| [ | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_zero, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_zero, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| top = _np.array( | ||
| [ | ||
| [v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_one, v_one], | ||
| [-v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| [v_zero, v_one], | ||
| ] | ||
| ) | ||
| left = _np.array( | ||
| [ | ||
| [v_zero, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| bottom_left = _np.array( | ||
| [ | ||
| [-v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_zero, -v_outer_c_h + v_one_half], | ||
| [-v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_zero], | ||
| ] | ||
| ) | ||
| top_left = _np.array( | ||
| [ | ||
| [v_zero, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_one, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| left = _np.array( | ||
| [ | ||
| [v_zero, -v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_zero, v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| ] | ||
| ) | ||
| bottom = _np.array( | ||
| [ | ||
| [v_one, v_zero, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_zero, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_zero, v_one], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_zero, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| top_left = _np.array( | ||
| [ | ||
| [v_zero, v_outer_c_h + v_one_half], | ||
| [-v_h_void + v_one_half, v_inner_c_h + v_one_half], | ||
| [v_zero, v_one], | ||
| [-v_inner_c_h + v_one_half, v_h_void + v_one_half], | ||
| ] | ||
| ) | ||
| bottom_right = _np.array( | ||
| [ | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_zero, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_zero, v_one], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| bottom = _np.array( | ||
| [ | ||
| [v_outer_c_h + v_one_half, v_zero], | ||
| [v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [-v_outer_c_h + v_one_half, v_zero], | ||
| [-v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| ] | ||
| ) | ||
| elif closure == "y_max": | ||
| # set points: | ||
| right = _np.array( | ||
| [ | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| bottom_right = _np.array( | ||
| [ | ||
| [v_inner_c_h + v_one_half, -v_h_void + v_one_half], | ||
| [v_outer_c_h + v_one_half, v_zero], | ||
| [v_h_void + v_one_half, -v_inner_c_h + v_one_half], | ||
| [v_one, -v_outer_c_h + v_one_half], | ||
| ] | ||
| ) | ||
| right_top = _np.array( | ||
| [ | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_one, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| spline_list.append(_Bezier(degrees=[1, 1], control_points=right)) | ||
| top = _np.array( | ||
| [ | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, v_one, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_one, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, v_one, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_one, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| spline_list.append(_Bezier(degrees=[1, 1], control_points=right_top)) | ||
| bottom_left = _np.array( | ||
| [ | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| spline_list.append(_Bezier(degrees=[1, 1], control_points=bottom)) | ||
| left = _np.array( | ||
| [ | ||
| [v_zero, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, -v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| spline_list.append(_Bezier(degrees=[1, 1], control_points=bottom_left)) | ||
| top_left = _np.array( | ||
| [ | ||
| [v_zero, v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_one, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_zero, v_outer_c_h + v_one_half, v_one], | ||
| [ | ||
| -v_h_void + v_one_half, | ||
| v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_zero, v_one, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| spline_list.append(_Bezier(degrees=[1, 1], control_points=left)) | ||
| bottom = _np.array( | ||
| [ | ||
| [v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [-v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| -v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| ] | ||
| ) | ||
|
|
||
| spline_list.append(_Bezier(degrees=[1, 1], control_points=top_left)) | ||
| bottom_right = _np.array( | ||
| [ | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_zero], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_zero, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_zero], | ||
| [ | ||
| v_inner_c_h + v_one_half, | ||
| -v_h_void + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_outer_c_h + v_one_half, v_zero, v_one], | ||
| [ | ||
| v_h_void + v_one_half, | ||
| -v_inner_c_h + v_one_half, | ||
| v_one, | ||
| ], | ||
| [v_one, -v_outer_c_h + v_one_half, v_one], | ||
| ] | ||
| ) | ||
|
|
||
| spline_list.append(_Bezier(degrees=[1, 1], control_points=top)) | ||
| for control_points in [ | ||
| right, | ||
| right_top, | ||
| bottom, | ||
| bottom_left, | ||
| left, | ||
| top_left, | ||
| top, | ||
| bottom_right, | ||
| ]: | ||
| spline_list.append( | ||
| _Bezier(degrees=[1, 1, 1], control_points=control_points) | ||
| ) | ||
|
|
||
| spline_list.append( | ||
| _Bezier(degrees=[1, 1], control_points=bottom_right) | ||
| ) | ||
| if i_derivative == 0: | ||
| splines = spline_list.copy() | ||
| else: | ||
| derivatives.append(spline_list) | ||
|
|
||
| return (spline_list, None) | ||
| return (splines, derivatives) |
There was a problem hiding this comment.
Add validation for closure parameter to prevent undefined variables.
The code checks if closure is None but doesn't validate that it's one of the expected values. If an invalid closure string is passed, the variables used in the loop at lines 1235-1243 won't be defined, causing a runtime error.
Add validation after the None check:
if closure is None:
raise ValueError("No closing direction given")
+
+ if closure not in self._closure_directions:
+ raise ValueError(
+ f"Invalid closure direction: {closure}. "
+ f"Must be one of {self._closure_directions}"
+ )
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 1236-1236: Possibly using variable 'right' before assignment
(E0606)
[error] 1237-1237: Possibly using variable 'right_top' before assignment
(E0606)
[error] 1238-1238: Possibly using variable 'bottom' before assignment
(E0606)
[error] 1239-1239: Possibly using variable 'bottom_left' before assignment
(E0606)
[error] 1240-1240: Possibly using variable 'left' before assignment
(E0606)
[error] 1241-1241: Possibly using variable 'top_left' before assignment
(E0606)
[error] 1242-1242: Possibly using variable 'top' before assignment
(E0606)
[error] 1243-1243: Possibly using variable 'bottom_right' before assignment
(E0606)
🤖 Prompt for AI Agents
In splinepy/microstructure/tiles/hollow_octagon_extrude.py around lines 273 to
1254, the closure parameter is only checked for None but not validated against
expected values like "x_min", "x_max", "y_min", or "y_max". To fix this, add a
validation step after the None check that raises a ValueError if closure is not
one of the allowed strings. This ensures all variables used later are properly
defined and prevents runtime errors from invalid closure inputs.
Overview
Addressing issue #392: this should add tests related to all microstructure- and microtiles-related code.
Addressing issue #458: some changes to the code of the microtiles and possibly the microstructure are needed in order to satisfy the tests.
Additionally, the code's layout for the tiles is unified, in particular:
create_tile()and_closing_tile()Checklist of what to test
Summary by CodeRabbit
Summary by CodeRabbit
New Features
HollowOctagonExtrude,HollowOctagon,Cross2D,Cross3D, andCross3DLinear.create_tileand closing tile methods to support parameter sensitivities and derivative computations.Bug Fixes
Tests