NF: All shape actor based on primitive

[ManishReddyR]

Hello,
Tried create the actor for Tetrahedron and also added few of the uni tests cases.


Thank You
Manish Rakasi

[skoudoro]

Hi @ManishReddyR,

• Can you rebase this PR on the top of the v2 branch (it avoids to see 15 commits)
• Can you do all of them in one PR? update this PR with all shape actor based on primitive. easy to review. I think it would be better than one by one. in this specific case.

thank you

[ManishReddyR]

Tried adding actor for icosahedron.

Thank you
Manish Rakasi

[skoudoro]

I see you requested a review @ManishReddyR. Thank you for the update.

What about superquadric, rectangle, arrow, cone, triangularprism, rhombicuboctahedron, pentagonalprism, octagonalprism, tetrahedron, icosahedron, star?

[skoudoro]

keep doing 1 commit per primitives shape, this is great and easier to review.

Thanks

[skoudoro]

See my comment above, many shapes are missing.

Otherwise, it looks good as it is now

[ManishReddyR]

I see you requested a review @ManishReddyR. Thank you for the update.

What about superquadric, rectangle, arrow, cone, triangularprism, rhombicuboctahedron, pentagonalprism, octagonalprism, tetrahedron, icosahedron, star?

working on it, will complete all of them by end of this weekend. And will update you.
Thanks for your review.

[ManishReddyR]

Added the actor for rhombicuboctahedron, and also added the uni tests.

Thank You

[ManishReddyR]

Added the actor for the triangularprism, And also added the uni tests for triangularprism actor.

Thank you

[ManishReddyR]

Added actor for pentagonalprism, and added uni tests for pentagonalprism.

Thank you

[ManishReddyR]

Added the actor for octogonal prism, and also added uni tests for octogonal prism actor.

Thank you

[ManishReddyR]

Added actor for arrow and also added uni tests for arrow.

Thank you

[ManishReddyR]

Added actor for superquadric and also added uni tests for superquadric.

Thank you

[ManishReddyR]

Hello @skoudoro ,

Can you review the actors, which I have added.

Thank you

[skoudoro]

Hi @ManishReddyR,

I will look at this on Friday. Thank you for working on this.

Can you update your tests ? they seems repetitive. Look here for an example on how you could refactor all of this:

fury/fury/tests/test_primitive.py

Lines 9 to 25 in 337f4b3

	def test_vertices_primitives():
	# Tests the default vertices of all the built in primitive shapes.
	l_primitives = [
	(fp.prim_square, (4, 3), -0.5, 0.5, 0),
	(fp.prim_box, (8, 3), -0.5, 0.5, 0),
	(fp.prim_tetrahedron, (4, 3), -0.5, 0.5, 0),
	(fp.prim_star, (10, 3), -3, 3, -0.0666666666),
	(fp.prim_rhombicuboctahedron, (24, 3), -0.5, 0.5, 0),
	(fp.prim_frustum, (8, 3), -0.5, 0.5, 0),
	]
	for func, shape, e_min, e_max, e_mean in l_primitives:
	vertices, _ = func()
	npt.assert_equal(vertices.shape, shape)
	npt.assert_almost_equal(np.mean(vertices), e_mean)
	npt.assert_equal(vertices.min(), e_min)
	npt.assert_equal(vertices.max(), e_max)

[maharshi-gor]

Hi @ManishReddyR Thank you for the work.

@skoudoro should we use only npt for the assertions? I see we have used mixture of assert and npt assert. If not, any specific reason for the same?

[skoudoro]

@skoudoro should we use only npt for the assertions? I see we have used mixture of assert and npt assert. If not, any specific reason for the same?

it is fine, as soon as they are explicit enough. assert do no handle array so npt assert is needed

[ManishReddyR]

Added actor for cone, and also uni tests for the cone actors.

Thank you

[ManishReddyR]

Added actor for star, and also added uni tests for star.

I have added almost all the actors based on the primitive. Now working on refactoring of the test cases.

Thank you

[skoudoro]

Overall, it is good,

see the comment below to update. Waiting for the tests refactorization also. Thank @ManishReddyR

[ManishReddyR]

Hello @skoudoro ,
Thanks for your review, and also refactored the tests.