Test microstructure and -tiles #457
Conversation
WalkthroughThe changes encompass a series of updates across multiple files in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TileClass
participant ParameterManager
User->>TileClass: create_tile(parameters)
TileClass->>ParameterManager: validate_parameters(parameters)
ParameterManager-->>TileClass: parameters_validated
TileClass->>User: return created_tile
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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:
|
| for i_derivative in range(n_derivatives + 1): | ||
| if i_derivative == 0: | ||
| v_wall_thickness = parameters[0, 0] | ||
| v_zero = 0.0 | ||
| v_one_half = 0.5 | ||
| v_one = 1.0 | ||
| v_half_contact_length = contact_length * 0.5 | ||
| v_inner_half_contact_length = contact_length * v_wall_thickness | ||
| else: | ||
| v_wall_thickness = parameter_sensitivities[ | ||
| 0, 0, i_derivative - 1 | ||
| ] | ||
| ) | ||
| v_zero = 0.0 | ||
| v_one_half = 0.0 | ||
| v_one = 0.0 | ||
| v_half_contact_length = 0.0 | ||
| v_inner_half_contact_length = contact_length * v_wall_thickness |
There was a problem hiding this comment.
I think these are derivatives independent and constant. Why don't we put them outside the loop? And only for v_wall_thinkness is the formula different in each iteration. So, in theory, if v_inner_half_contact_length is under the if clause, only v_wall_thinkness needs to be in the if, and only these two need to be declared in the loop at all, the other can be declared before the loop.
Or am I missing something?
There was a problem hiding this comment.
These are constants/parameters for a specific microtile. In the code below these declarations are the descriptions of the patches which the microstructure is made out of. In every of the patches' description these constants are in there (e.g. v_inner_half_contact_length is in the definition of the first patch). In the first loop iteration the geometry is constructed out of these patches. The loop iterations afterwards describe the analytical derivatives of each patch w.r.t. the parameters. That's why we have to hardcode these values, once for the geometry in the first loop iteration, and once for the derivatives in the subsequent iterations. That's also why we have to redefine v_inner_half_contact_length in the second loop iteration (in the else clause), because it depends on v_wall_thickness which is different in the if clause and the else clause. Each patch is defined once and hence we have to redefine these constants
There was a problem hiding this comment.
Otherwise we would have to define each patch a second time, once for the geometry and once for the derivatives. Then we could define two separate e.g. v_inner_half_contact_lengths. But the additional definitions of these patches for the derivatives would add extra code and would lead to more difficulties in readability and modifiability
|
|
||
| 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.
| if i_derivative == 0: | ||
| splines = spline_list.copy() | ||
| else: | ||
| derivatives.append(spline_list) |
There was a problem hiding this comment.
derivatives are just a variation of wall thickness (only v_wall_thinkness is changed)?
There was a problem hiding this comment.
spline_list is different in each loop iteration. derivatives also change due to variations of v_one_half, v_one
| @@ -61,7 +65,9 @@ def _closing_tile( | |||
| if closure is None: | |||
| raise ValueError("No closing direction given") | |||
|
|
|||
| self.check_params(parameters) | |||
| # TODO: parameters are not implemented, therefore do not check params | |||
| if parameters is not None: | |||
There was a problem hiding this comment.
Why is there first a check if the parameters are None and not by all the other tiles?
There was a problem hiding this comment.
The parameters for this tile are not implemented. Right now the code does not reach these lines when parameters are defined, since def _closing_tile is run after the creation of the other tiles. It would raise a not implemented error in the def create_tile() before that. So technically, this line is wrong, but it never reaches that line.
Thanks for catching that. Should I change that?
| face_max_area > 1.0 - EPS | ||
| ), f"The closure of the {closure_direction}_max surface is not complete" | ||
|
|
||
| for tile_class in all_tile_classes: |
There was a problem hiding this comment.
Instead of this for loop use use this as the function handle.
@pytest.parametrize("tile_class", all_tile_classes)
def test_closing_face(tile_class):
There was a problem hiding this comment.
The same for the other functions here. That would create a test_case for each tile_class so it is much easier to distinguish the relevant tile_class if an error is thrown.
| for skip_tile in skip_tiles: | ||
| if tile_class is skip_tile: | ||
| skip_tile_class = True | ||
| break | ||
| if skip_tile_class: | ||
| continue |
There was a problem hiding this comment.
| for skip_tile in skip_tiles: | |
| if tile_class is skip_tile: | |
| skip_tile_class = True | |
| break | |
| if skip_tile_class: | |
| continue | |
| if tile_class in skip_tiles: | |
| skip_tile_class = True | |
| continue |
This should work.
| for skip_tile in skip_tiles: | ||
| if tile_class is skip_tile: | ||
| skip_tile_class = True | ||
| break | ||
| if skip_tile_class: | ||
| continue |
| v_zero = 0.0 | ||
| v_one_half = 0.0 | ||
| v_one = 0.0 | ||
| v_h_void = parameter_sensitivities[0, 0, i_derivative - 1] |
There was a problem hiding this comment.
You are checking v_h_void before that it is in between 0 and 0.5 what happens if the sensitivities make the value escape this range?
| @@ -32,6 +32,11 @@ class InverseCross3D(_TileBase): | |||
| ] | |||
| ) | |||
| _n_info_per_eval_point = 1 | |||
| # TODO: implemented sensitivities are not correct | |||
| _sensitivities_implemented = True | |||
| _closure_directions = ["z_min", "z_max"] | |||
There was a problem hiding this comment.
Why is this limited to these? just bc. we have not added them here or is there a deeper reason?
There was a problem hiding this comment.
If you are talking about the closure directions, then yes. We have not added them
| # Auxiliary values | ||
| fill_height_aux = filling_height | ||
| sep_distance_aux = separator_distance | ||
| inv_filling_height = 1.0 - filling_height | ||
| ctps_mid_height_top = (1 + filling_height) * 0.5 | ||
| ctps_mid_height_bottom = 1.0 - ctps_mid_height_top | ||
| center_width = 1.0 - 2 * boundary_width | ||
| r_center = center_width * 0.5 | ||
| half_r_center = (r_center + 0.5) * 0.5 | ||
| aux_column_width = 0.5 - 2 * (0.5 - separator_distance) | ||
| v_zero = 0.0 | ||
| v_one_half = 0.5 | ||
| v_one = 1.0 | ||
| if closure == "z_min": | ||
| branch_thickness = parameters.flatten()[5] | ||
| elif closure == "z_max": | ||
| branch_thickness = parameters.flatten()[4] | ||
| else: | ||
| fill_height_aux = 0.0 | ||
| sep_distance_aux = 0.0 | ||
| inv_filling_height = 0.0 | ||
| ctps_mid_height_top = 0.0 | ||
| ctps_mid_height_bottom = 0.0 | ||
| center_width = 0.0 | ||
| r_center = 0.0 | ||
| half_r_center = 0.0 | ||
| aux_column_width = 0.0 | ||
| v_zero, v_one_half, v_one = [0.0] * 3 | ||
| if closure == "z_min": | ||
| branch_thickness = parameter_sensitivities.flatten()[5] | ||
| elif closure == "z_max": | ||
| branch_thickness = parameter_sensitivities.flatten()[4] |
There was a problem hiding this comment.
Many of these parameters are sensitivities independent. Could we take them out of the if statement so that they are not doubled? Same as the other files?
There was a problem hiding this comment.
Same as the others: every patch is defined using these variables. For the derivatives these variables have to be redefined in order to keep the patches' definition the same
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.
Checklist of what to test
Summary by CodeRabbit
New Features
Bug Fixes
Tests