Add primitive and actor for pentagonal prism with test #474

mehabhalodiya · 2021-07-27T11:37:18Z

I made a primitive for pentagonal prism and created an actor for the same.
Added tests for actor and primitive too.

Preview:

pep8speaks · 2021-07-27T11:37:22Z

Hello @mehabhalodiya! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file fury/actor.py:

Line 1722:69: W291 trailing whitespace

Comment last updated at 2021-09-03 09:52:14 UTC

SunTzunami · 2021-07-27T14:37:56Z

fury/tests/test_primitive.py

+    onec = (five - 1) / 4.0
+    twoc = (five + 1) / 4.0
+    sone = (math.sqrt(10 + (2 * five))) / 4.0
+    stwo = (math.sqrt(10 - (2 * five))) / 4.0


Hi @mehabhalodiya . Thanks for the PR.
More descriptive variable names will be better here.

Can you suggest me some descriptive variable names?
I am asking because everywhere it's like (u, v), (c1, s1), etc.

Please address the centering query that Serge has raised, if the prism is centered properly, I'll let you know some better variable names.

skoudoro

Hi @mehabhalodiya,

Thank you so much for doing that!

Can you add a screenshot of this actor on your PR. When you add a screenshot, please, add also the actor.axes(). Your pentagonal prism should be centered in (0,0,0) which is not currently the case. Adding the xyz axes will help you to see it.

Otherwise, see below some of my comments.

Thank you

skoudoro · 2021-07-27T16:29:43Z

fury/actor.py

+    ----------
+    centers : ndarray, shape (N, 3)
+        Pentagonal prism positions
+    directions : ndarray, shape (N, 3)


ndarray, shape (N, 3), optional

skoudoro · 2021-07-27T16:29:55Z

fury/actor.py

+        Pentagonal prism positions
+    directions : ndarray, shape (N, 3)
+        The orientation vector of the pentagonal prism.
+    colors : ndarray (N,3) or (N, 4) or tuple (3,) or tuple (4,)


, optional

skoudoro · 2021-07-27T16:33:26Z

fury/primitive.py

@@ -626,6 +626,55 @@ def prim_triangularprism():
    return vertices, triangles


+def prim_pentagonalprism():
+    """Return vertices and triangle for an pentagonal prism.


typo: triangles

typo: a instead of "an"

skoudoro · 2021-07-27T16:35:20Z

fury/primitive.py

+    vertices = np.array([[-twoc, stwo, -1],
+                         [onec, sone, -1],
+                         [1, 0, -1],
+                         [onec, -sone, -1],
+                         [-twoc, -stwo, -1],
+                         [-twoc, stwo, 1],
+                         [onec, sone, 1],
+                         [1, 0, 1],
+                         [onec, -sone, 1],
+                         [-twoc, -stwo, 1]])


are you sure that your coordinates are centered with (0,0,0)?

Same comment as @SunTzunami, use more descriptive variable names.

skoudoro · 2021-07-27T16:36:39Z

fury/tests/test_primitive.py

+    sone = (math.sqrt(10 + (2 * five))) / 4.0
+    stwo = (math.sqrt(10 - (2 * five))) / 4.0
+    npt.assert_equal(vertices.shape, shape)
+    npt.assert_equal(np.mean(vertices), 0)


this test is failing because your primitive is not centered.

Can you please help me to get it centered?
How can I do?

Please, provide a screenshot of your actor with axes actor and then, we can have an idea and help you

Done!
Updated! Please have a look @skoudoro @SunTzunami

Please provide a better visual where the axes are visible, see this for reference.

skoudoro · 2021-07-27T16:36:50Z

fury/tests/test_primitive.py

+    sone = (math.sqrt(10 + (2 * five))) / 4.0
+    stwo = (math.sqrt(10 - (2 * five))) / 4.0
+    npt.assert_equal(vertices.shape, shape)
+    npt.assert_equal(np.mean(vertices), 0)


this test is failing because your primitive is not centered.

skoudoro · 2021-07-29T10:36:37Z

Thanks @SunTzunami for helping! Could you help @devmessias and @antrikshmisri? thank you in advance

mehabhalodiya · 2021-07-29T14:27:24Z

I updated the primitive:

Pentagonal prism

@skoudoro @SunTzunami Requesting you to have a look!

Let me know if it's alright, so that I can push the changes requested.

skoudoro · 2021-07-29T15:02:03Z

Good job @mehabhalodiya, it looks much better! I did not try it yet, I will do it later but just make sure that your primitive is contained in the unit axes (actor.axes(scales=1)) and it will be perfect. your PR is close to be done

devmessias · 2021-07-29T19:57:14Z

fury/tests/test_primitive.py

+    # Testing the default vertices of the primitive pentagonal prism.
+    vertices, _ = fp.prim_pentagonalprism()
+    shape = (10, 3)
+    five = (float('{:.7f}'.format(math.sqrt(5))))


Hi. you should use round instead of casting the number into a string and converting back into a float number

>>> import math >>> (float('{:.7f}'.format(math.sqrt(5)))) 2.236068 >>> round(math.sqrt(5), 7) 2.236068

I believe the following test is more suitable. In addition, this will work.

def test_vertices_primitives_pentagonalprism(): # Testing the default vertices of the primitive pentagonal prism. vertices, _ = fp.prim_pentagonalprism() lower_face = vertices[:, 0:2][0:5, ] upper_face = vertices[:, 0:2][5:10, ] centroid_upper = np.mean(upper_face, 0) centroid_lower = np.mean(lower_face, 0) shape = (10, 3) npt.assert_equal(vertices.shape, shape) # This test will check whether the z-axis vertex dispersion is correct npt.assert_almost_equal(np.mean(vertices[:, 2]), 0) #check if the centroid of the upper face is at the origin npt.assert_almost_equal(centroid_upper, np.array([0, 0])) # check if the centroid of the lower face is at the origin npt.assert_almost_equal(centroid_lower, np.array([0, 0]))

devmessias · 2021-07-29T19:59:12Z

fury/primitive.py

+    """
+    # Local variable to represent the square root of five rounded
+    # to 7 decimal places
+    five = float('{:.7f}'.format(math.sqrt(5)))


Same comment about round method

Hi. you should use round instead of casting the number into a string and converting back into a float number

>>> import math >>> (float('{:.7f}'.format(math.sqrt(5)))) 2.236068 >>> round(math.sqrt(5), 7) 2.236068

Are any special reason for rounding sqrt(5)? If not you should remove this and use just five = math.sqrt(5)

fury/actor.py

devmessias · 2021-08-02T11:59:26Z

fury/primitive.py

+    """
+    # Local variable to represent the square root of five rounded
+    # to 7 decimal places
+    five = float('{:.7f}'.format(math.sqrt(5)))


Are any special reason for rounding sqrt(5)? If not you should remove this and use just five = math.sqrt(5)

devmessias · 2021-08-02T12:09:12Z

fury/tests/test_primitive.py

+    # Testing the default vertices of the primitive pentagonal prism.
+    vertices, _ = fp.prim_pentagonalprism()
+    shape = (10, 3)
+    five = (float('{:.7f}'.format(math.sqrt(5))))


I believe the following test is more suitable. In addition, this will work.

def test_vertices_primitives_pentagonalprism(): # Testing the default vertices of the primitive pentagonal prism. vertices, _ = fp.prim_pentagonalprism() lower_face = vertices[:, 0:2][0:5, ] upper_face = vertices[:, 0:2][5:10, ] centroid_upper = np.mean(upper_face, 0) centroid_lower = np.mean(lower_face, 0) shape = (10, 3) npt.assert_equal(vertices.shape, shape) # This test will check whether the z-axis vertex dispersion is correct npt.assert_almost_equal(np.mean(vertices[:, 2]), 0) #check if the centroid of the upper face is at the origin npt.assert_almost_equal(centroid_upper, np.array([0, 0])) # check if the centroid of the lower face is at the origin npt.assert_almost_equal(centroid_lower, np.array([0, 0]))

mehabhalodiya · 2021-08-03T12:56:41Z

Hey @skoudoro, I have updated, PTAL. Thanks!

codecov · 2021-08-03T15:05:46Z

Codecov Report

Merging #474 (53cdb53) into master (f376819) will increase coverage by 0.54%.
The diff coverage is 100.00%.

❗ Current head 53cdb53 differs from pull request most recent head 49e2a5c. Consider uploading reports for the commit 49e2a5c to get more accurate results

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   88.41%   88.96%   +0.54%     
==========================================
  Files          31       33       +2     
  Lines        6519     7070     +551     
  Branches      782      832      +50     
==========================================
+ Hits         5764     6290     +526     
- Misses        535      547      +12     
- Partials      220      233      +13

Impacted Files	Coverage Δ
fury/actor.py	`89.44% <100.00%> (+0.39%)`	⬆️
fury/primitive.py	`92.13% <100.00%> (+0.35%)`	⬆️
fury/utils.py	`85.85% <0.00%> (-0.70%)`	⬇️
fury/layout.py	`100.00% <0.00%> (ø)`
fury/shaders/__init__.py	`100.00% <0.00%> (ø)`
fury/ui/tests/test_helpers.py	`100.00% <0.00%> (ø)`
fury/actors/tests/test_peak.py	`97.95% <0.00%> (ø)`
fury/actors/peak.py	`90.00% <0.00%> (ø)`
fury/shaders/base.py	`91.54% <0.00%> (+0.16%)`	⬆️
... and 2 more

SunTzunami · 2021-08-06T12:13:12Z

@mehabhalodiya please make the following changes (refer to the image below):

Make H = L.
Make the edge pass through the y axis (there's gap between the edge and the y axis).

devmessias · 2021-08-06T13:18:56Z

@mehabhalodiya please make the following changes (refer to the image below):

Make H = L.

Make the edge pass through the y axis (there's gap between the edge and the y axis).

I don't know if H=L is mandatory. Maybe is better H to be a parameter.

mehabhalodiya · 2021-08-06T13:19:34Z

@SunTzunami How it's possible to pass two axes through edges?
One will pass through the edge, second one from besides the edge and third one through the origin!

SunTzunami · 2021-08-06T13:28:19Z

@mehabhalodiya Read my comment once again, I mentioned that the edge must pass through the y axis (in your case, it's passing through the x-axis), manipulate the coordinates in the primitive to ensure that it passes through the y-axis.
See here in triangular prism:

The edge is clearly passing through the y-axis. I'm emphasizing on this point to ensure that all primitives are similarly centered.

fury/actor.py

devmessias · 2021-08-06T13:35:31Z

fury/actor.py

+    >>> dirs = np.random.rand(3, 3)
+    >>> colors = np.random.rand(3, 3)
+    >>> scales = np.random.rand(3, 1)
+    >>> actor = actor.pentagonalprism(centers, dirs, colors, scales)


You should avoid to overwrite an import. In your case you have the same name for actor, which is a module from FURY, and the pentagonalprism. I think the following is better

from fury import window, actor .... actor_pentagonal = actor.pentagonalprism(centers, dirs, colors, scales) scene.add(actor_pentagonal)

SunTzunami · 2021-08-06T13:38:03Z

@devmessias
I'm not sure if H should be added as a parameter as H (i.e. height) can be manipulated by using scales parameter of the actor. I'm insisting on keeping H = L as all other prisms have H = L.

devmessias · 2021-08-06T13:45:35Z

@devmessias
I'm not sure if H should be added as a parameter as H (i.e. height) can be manipulated by using scales parameter of the actor. I'm insisting on keeping H = L as all other prisms have H = L.

Scale just scales all the vertices. Using H as a parameter you can have things like that. But is just a matter of choice.

In addition is not so hard to add this

on actor.py

def pentagonalprism(
    centers, directions=(1, 0, 0), colors=(1, 0, 0), scales=1, height=1):
 ....
    verts, faces = fp.prim_pentagonalprism(height)

on primitives.py

def prim_pentagonalprism(height=1.0):
   ....
   vertices[:,2] = height*vertices[:,2]
   triangles = np.array([[9, 5, 4],...
   ```

SunTzunami · 2021-08-06T13:50:09Z

@devmessias
I'm not sure if H should be added as a parameter as H (i.e. height) can be manipulated by using scales parameter of the actor. I'm insisting on keeping H = L as all other prisms have H = L.

Scale just scales all the vertices. Using H as a parameter you can have things like that. But is just a matter of choice.

In addition is not so hard to add this

on actor.py
def pentagonalprism(
    centers, directions=(1, 0, 0), colors=(1, 0, 0), scales=1, height=1):
 ....
    verts, faces = fp.prim_pentagonalprism(height)   
on primitives.py
   vertices[:,2] = height*vertices[:,2]
   triangles = np.array([[9, 5, 4],...

Makes sense.

mehabhalodiya · 2021-08-07T09:33:56Z

@SunTzunami @devmessias I have updated! Have a look:

Pentagonal prism

Let me know if it's alright now!

SunTzunami · 2021-08-07T15:43:23Z

Hi @mehabhalodiya ! The figure looks pretty similar to what was required. Thank you for the update. I had one doubt: Have you set H=L (i.e. height of the prism = length of the side)? Otherwise it looks good to me.
Don't forget to update your tests and please push your changes. Pretty good progress so far!

mehabhalodiya · 2021-08-08T08:49:48Z

@SunTzunami PTAL!
Thank you

devmessias · 2021-08-08T12:48:54Z

@SunTzunami @devmessias I have updated! Have a look:

Pentagonal prism

Let me know if it's alright now!

Hi @mehabhalodiya, thanks for addressing my suggestions. I see that you have fixed the example in the docstring and now H is a parameter. This PR looks good to me.

mehabhalodiya · 2021-08-18T15:15:42Z

Can anyone please let me know how to make: "The edges of rectangle = The edges of regular pentagon"?

I have tried doing:

vertices = np.array([[stwo/2, twoc/2, -0.294],
                     [sone/2, -onec/2, -0.294],
                     [0, -1/2, -0.294],
                     [-sone/2, -onec/2, -0.294],
                     [-stwo/2, twoc/2, -0.294],
                     [stwo/2, twoc/2, 0.294],
                     [sone/2, -onec/2, 0.294],
                     [0, -1/2, 0.294],
                     [-sone/2, -onec/2, 0.294],
                     [-stwo/2, twoc/2, 0.294]])  

print(math.sqrt((((sone/2)-(stwo/2))**2) + (((-onec/2)-(twoc/2))**2))/2)

But looks like it's not working from my side!
Please help me.

skoudoro

I don't know if H=L is mandatory. Maybe is better H to be a parameter.

H=L does not need to be mandatory @SunTzunami.

I'm not sure if H should be added as a parameter as H (i.e. height) can be manipulated by using scales parameter of the actor. I'm insisting on keeping H = L as all other prisms have H = L.

I agree with @SunTzunami. No need to have H as parameter. scales does the job. Otherwise, can you clarifies what will be the differences between H and scales? Let's keep it simple.

Hi @mehabhalodiya,

Thank you for this good job and apologies for the late reply. Your PR is in a good shape and no need to add more complex. See below for some comments and it will be ready to go

skoudoro · 2021-09-02T19:16:27Z

fury/actor.py

@@ -1719,6 +1719,48 @@ def triangularprism(centers, directions=(1, 0, 0), colors=(1, 0, 0),
    return tri_actor


+def pentagonalprism(centers, directions=(1, 0, 0), colors=(1, 0, 0),
+                    scales=1, height=1):


you can remove height, let's keep it simple

skoudoro · 2021-09-02T19:17:42Z

fury/primitive.py

@@ -626,6 +626,55 @@ def prim_triangularprism():
    return vertices, triangles


+def prim_pentagonalprism(height=1.0):


you can remove height

skoudoro · 2021-09-02T19:18:28Z

fury/primitive.py

+                         [0, -1/2, 1.84],
+                         [-sone/2, -onec/2, 1.84],
+                         [-stwo/2, twoc/2, 1.84]])
+    vertices[:, 2] = height*vertices[:, 2]


pep8: Can you add spaces between multiplication here. Thank you!

If I am removing height then how can I add vertices[:, 2] = height * vertices[:, 2] line?

Should I consider removing vertices[:, 2] = height*vertices[:, 2] line?

skoudoro · 2021-09-02T19:18:57Z

fury/tests/test_primitive.py

+def test_vertices_primitives_pentagonalprism():
+    # Testing the default vertices of the primitive pentagonal prism.
+    vertices, _ = fp.prim_pentagonalprism()
+    lower_face = vertices[:, 0:2][0:5, ]
+    upper_face = vertices[:, 0:2][5:10, ]
+    centroid_upper = np.mean(upper_face, 0)
+    centroid_lower = np.mean(lower_face, 0)
+    shape = (10, 3)
+    npt.assert_equal(vertices.shape, shape)
+    # This test will check whether the z-axis vertex dispersion is correct
+    npt.assert_almost_equal(np.mean(vertices[:, 2]), 0)
+    # check if the centroid of the upper face is at the origin
+    npt.assert_almost_equal(centroid_upper, np.array([0, 0]))
+    # check if the centroid of the lower face is at the origin
+    npt.assert_almost_equal(centroid_lower, np.array([0, 0]))


mehabhalodiya · 2021-09-03T06:46:53Z

Hey @skoudoro , all the proposed changes have been added. PTAL.

skoudoro

Thank you for the quick update @mehabhalodiya!

Looks good! See below the last comment before merging

skoudoro · 2021-09-03T09:34:43Z

fury/primitive.py

+    vertices = np.array([[stwo/2, twoc/2, -1.84],
+                         [sone/2, -onec/2, -1.84],
+                         [0, -1/2, -1.84],
+                         [-sone/2, -onec/2, -1.84],
+                         [-stwo/2, twoc/2, -1.84],
+                         [stwo/2, twoc/2, 1.84],
+                         [sone/2, -onec/2, 1.84],
+                         [0, -1/2, 1.84],
+                         [-sone/2, -onec/2, 1.84],
+                         [-stwo/2, twoc/2, 1.84]])


instead of 1.84 on z-axes, can you use 0.5. You might need to update the tests accordingly.

mehabhalodiya · 2021-09-03T09:53:51Z

Hey @skoudoro , minor change of z-axes coordinates have been added. PTAL.

skoudoro · 2021-09-03T10:08:10Z

Great! Thank you for this work and for this new actor!

Thanks also to @SunTzunami and @devmessias for the nice and complete review.

merging!

Add primitive and actor for pentagonal prism with test

0513549

mehabhalodiya added 3 commits July 27, 2021 17:20

fix pep8

3f58750

pep8

58e4a9d

pep8

ff8aa2a

SunTzunami reviewed Jul 27, 2021

View reviewed changes

skoudoro reviewed Jul 27, 2021

View reviewed changes

skoudoro added the type:New Feature label Jul 27, 2021

skoudoro requested review from devmessias and antrikshmisri July 29, 2021 10:35

devmessias reviewed Jul 29, 2021

View reviewed changes

devmessias suggested changes Aug 2, 2021

View reviewed changes

mehabhalodiya added 2 commits August 3, 2021 18:04

Fix test failing

0801837

Removed test scene file

34bee26

devmessias suggested changes Aug 6, 2021

View reviewed changes

Update the primitive

45a498a

Fix PEP 8 issues

8318328

skoudoro reviewed Sep 2, 2021

View reviewed changes

Remove height as parameter

53cdb53

skoudoro reviewed Sep 3, 2021

View reviewed changes

Update z-axes coordinates

49e2a5c

skoudoro merged commit bba8f5f into fury-gl:master Sep 3, 2021

		@@ -626,6 +626,55 @@ def prim_triangularprism():
		return vertices, triangles


		def prim_pentagonalprism(height=1.0):

Add primitive and actor for pentagonal prism with test #474

Add primitive and actor for pentagonal prism with test #474

Conversation

mehabhalodiya commented Jul 27, 2021 • edited

pep8speaks commented Jul 27, 2021 • edited

Comment last updated at 2021-09-03 09:52:14 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehabhalodiya Jul 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoudoro commented Jul 29, 2021

mehabhalodiya commented Jul 29, 2021

skoudoro commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehabhalodiya commented Aug 3, 2021

codecov bot commented Aug 3, 2021 • edited

Codecov Report

SunTzunami commented Aug 6, 2021

devmessias commented Aug 6, 2021

mehabhalodiya commented Aug 6, 2021

SunTzunami commented Aug 6, 2021 • edited

devmessias Aug 6, 2021 • edited

Choose a reason for hiding this comment

SunTzunami commented Aug 6, 2021

devmessias commented Aug 6, 2021 • edited

SunTzunami commented Aug 6, 2021

mehabhalodiya commented Aug 7, 2021 • edited

SunTzunami commented Aug 7, 2021

mehabhalodiya commented Aug 8, 2021

devmessias commented Aug 8, 2021

mehabhalodiya commented Aug 18, 2021

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehabhalodiya commented Sep 3, 2021

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehabhalodiya commented Sep 3, 2021

skoudoro commented Sep 3, 2021

mehabhalodiya commented Jul 27, 2021 •

edited

pep8speaks commented Jul 27, 2021 •

edited

mehabhalodiya Jul 28, 2021 •

edited

codecov bot commented Aug 3, 2021 •

edited

SunTzunami commented Aug 6, 2021 •

edited

devmessias Aug 6, 2021 •

edited

devmessias commented Aug 6, 2021 •

edited

mehabhalodiya commented Aug 7, 2021 •

edited