NF: adding primitive stars, 3D stars, rhombi. #170
Conversation
|
Hello @lidonohu! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-03-05 22:16:39 UTC |
Codecov Report
@@ Coverage Diff @@
## master #170 +/- ##
==========================================
- Coverage 88% 87.79% -0.21%
==========================================
Files 17 17
Lines 4184 4196 +12
Branches 518 520 +2
==========================================
+ Hits 3682 3684 +2
- Misses 365 375 +10
Partials 137 137
|
| npt.assert_equal(vertices.shape, shape) | ||
| npt.assert_equal(np.mean(vertices), 0) | ||
| npt.assert_equal(vertices.min(), -.5) | ||
| npt.assert_equal(vertices.max(), 0.5) | ||
|
|
||
|
|
||
|
|
fury/primitive.py
Outdated
| @@ -5,7 +5,6 @@ | |||
| from scipy.spatial import ConvexHull | |||
| from scipy.spatial import transform | |||
|
|
|||
|
|
|||
I have made all the corrections that you suggested. Let me know in case of further changes. |
There was a problem hiding this comment.
Hi @lidonohu, thank you for the update!
You can see below some comments. As you can see below @ShreyasFadnavis and @lidonohu , there are many points that I do not understand and I think this why all tests are failing. I suppose, a rebase or an update from master will fix these problems
| big_vertices += big_centers | ||
|
|
||
| return big_vertices, big_triangles, big_colors, big_centers | ||
| return big_vertices, big_triangles, big_colors |
There was a problem hiding this comment.
Why do you remove big_centers ?
|
|
||
| """ | ||
| # duplicated vertices if needed | ||
| if not have_tiled_verts: | ||
| vertices = np.tile(vertices, (centers.shape[0], 1)) | ||
| big_vertices = vertices | ||
|
|
There was a problem hiding this comment.
Can you explain why do you remove big_vertices?
| @@ -117,18 +117,19 @@ def repeat_primitive(vertices, faces, centers, directions=(1, 0, 0), | |||
| Expanded triangles that composed our shape to duplicate | |||
| big_colors : ndarray | |||
| Expanded colors applied to all vertices/faces | |||
| big_centers : ndarray | |||
| Expanded centers for all vertices/faces | |||
There was a problem hiding this comment.
Can you explain why you remove this docstring ?
| # Get unit shape | ||
| unit_verts_size = vertices.shape[0] // centers.shape[0] | ||
| unit_triangles_size = faces.shape[0] | ||
|
|
||
| big_centers = np.repeat(centers, unit_verts_size, axis=0) | ||
| # apply centers position | ||
| big_vertices = vertices + big_centers |
There was a problem hiding this comment.
Same questions as above, I don't understand why you add this part? Please, can you explain ?
| @@ -163,20 +164,18 @@ def normalize_input(arr, arr_name=''): | |||
|
|
|||
| # update orientations | |||
| directions = normalize_input(directions, 'directions') | |||
| big_vertices -= big_centers | |||
| vertices coords that composed our rhombicuboctahedron | ||
| my_triangles: ndarray | ||
| Triangles that composed our rhombicuboctahedron | ||
| """ |
|
|
||
| for func, shape in l_primitives: | ||
| for func, shape, min, max, mean in l_primitives: |
There was a problem hiding this comment.
min, max, mean are predefined by python so it is better to avoid this variable name. Can you change them? Just put e_min, .... the e is for expected
|
All changes have been made in the new PR |
test file
add primitive