Bugfix mesh periodic #27
Conversation
Add the _is_periodic as a function in test_utils
This generate a test for octet truss considering two methods : The fuse of Shapes in one cq.Shape The cut of Phases to obtain an heterogeneous (i.e. one phase per cut cylinder)
Correct bug bounds_max[:, axis] -= 1 is redundant since bounds_min has been modified to test the same bounding boxes
minor fix : add fixtures for filenames
There was a problem hiding this comment.
This PR seems ok to me.
I do appreciate the added comments in mesh.py to help with understanding how meshPeriodic works
However I do believe that tests must be implemented for is_periodic as it seems to be an important function. I would first test it on a simple cube where I know the mesh to be periodic, then on another simple cube with an additional point on one face, and finally on another simple cube where one node is slightly shifted in regards to its corresponding node on the opposing face. I believe those test cases should cover all possibilities.
I understand the main goal of this PR was to fix bug #25 but I think meshPeriodic must be tested on a simpler box mesh prior to testing on a more complicated structure. By the way, why the two generation methods for the octet truss?
Finally, I'm not certain whether placing is_periodic in a test_utils kind of file is the most appropriate solution. Could it be a method in microgen's Mesh class?
microgen/mesh.py
Outdated
| for bounds_min, tag_min in _iter_bounding_boxes(minimum, maximum, eps): | ||
| # Translate the minmal bounds into the maximum value on axis surface |
Use numpy.typing syntax instead of hinting types in comments:
this replace the octetruss test by adding a simple box test
This adds a test on the is_periodic function
This reverts commit 3ac6d54.
Added a simple Box test
Allows to have a different center and cell size than the unit one
Added tests with non-unit RVE size
|
A commit has been added to correct the fact that the function was working for unit size cells only. This has been fixed and cells of different size and position have been added in the tests |
There was a problem hiding this comment.
The new tests look great! I suggested a few changes that are more related to style issues, and I noticed some inconsistencies in the formatting (there should be 2 spaces between the functions according to PEP8).
Maybe consider using black?
There was a problem hiding this comment.
I feel like this new module is not named correctly or is in the wrong place.
What about moving the is_periodic function to the microgen.periodic module?
There was a problem hiding this comment.
This is a good suggestion but since we are thinking about splitting micron somehow to handle either meshes or CAD geometry I think it is not recommended to place this into periodic at this time.
This module is temporary since it is scheduled to add a class that handle Box Meshes. The is_periodic function would naturally finds its place there
microgen/test_utils.py
Outdated
| zmin = np.min(nodes_coords[:, 2]) | ||
|
|
||
| # extract face nodes | ||
| left = np.where(np.abs(nodes_coords[:, 0] - xmin) < tol)[0] |
There was a problem hiding this comment.
Certainly a matter of preference, but the pattern np.where(...)[0] can be replaced by np.flatnonzero(...)
There was a problem hiding this comment.
This is an interesting thought but I do now how to handle the tolerance with np.flatnonzero(...)
changed maximum[axis] = _get_min(rve)[axis] to maximum[axis] = minimum[axis]
changed names left,.. to face_Xm,...
|
Since the comments have been adresses I think this PR is ready to be merged |
Overview
Fixes bug #25
Details
Essentially removed the line :
bounds_max[:, axis] -= 1 in mesh.pysincebounds_minhas been modifiedAdd test for an octet truss structure using two type of generation