Fixed warnings in test_utils.py #394

Shadow2121 · 2021-03-17T17:11:23Z

fixed warning in test_utils.py.

warning was: #317

fury/tests/test_utils.py::test_trilinear_interp
fury/tests/test_utils.py::test_trilinear_interp
  D:\fury-master\fury\tests\test_utils.py:133: DeprecationWarning: Converting `np.integer` or `np.signedinteger` to a dtype is deprecated. The current result is `np.dtype(np.int_)` which is not strictly correct. Note that the result depends on the system. To ensure stable results use may want to use `np.int64` or `np.int32`.
    x0 = x_indices.astype(np.integer)

fury/tests/test_utils.py::test_trilinear_interp
fury/tests/test_utils.py::test_trilinear_interp
  D:\fury-master\fury\tests\test_utils.py:134: DeprecationWarning: Converting `np.integer` or `np.signedinteger` to a dtype is deprecated. The current result is `np.dtype(np.int_)` which is not strictly correct. Note that the result depends on the system. To ensure stable results use may want to use `np.int64` or `np.int32`.
    y0 = y_indices.astype(np.integer)

fury/tests/test_utils.py::test_trilinear_interp
fury/tests/test_utils.py::test_trilinear_interp
  D:\fury-master\fury\tests\test_utils.py:135: DeprecationWarning: Converting `np.integer` or `np.signedinteger` to a dtype is deprecated. The current result is `np.dtype(np.int_)` which is not strictly correct. Note that the result depends on the system. To ensure stable results use may want to use `np.int64` or `np.int32`.
    z0 = z_indices.astype(np.integer)

It is not much but I noticed it so I just changed it.

The equation looks like this in tutorial of solar system visualization,

link - https://en.wikipedia.org/wiki/Orbital_period

It is not much but I noticed it so I just changed it.

codecov · 2021-03-17T17:16:54Z

Codecov Report

Merging #394 (7e11233) into master (58dff3a) will decrease coverage by 0.20%.
The diff coverage is 100.00%.

❗ Current head 7e11233 differs from pull request most recent head 2fb518f. Consider uploading reports for the commit 2fb518f to get more accurate results

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
- Coverage   89.08%   88.87%   -0.21%     
==========================================
  Files          23       23              
  Lines        5376     5393      +17     
  Branches      695      696       +1     
==========================================
+ Hits         4789     4793       +4     
- Misses        406      415       +9     
- Partials      181      185       +4

Impacted Files	Coverage Δ
fury/utils.py	`86.54% <100.00%> (ø)`
fury/__init__.py	`65.71% <0.00%> (-11.43%)`	⬇️
fury/ui.py	`88.54% <0.00%> (-0.46%)`	⬇️
fury/window.py	`78.75% <0.00%> (ø)`
fury/interactor.py	`96.96% <0.00%> (+0.02%)`	⬆️
fury/actor.py	`92.95% <0.00%> (+0.04%)`	⬆️
fury/primitive.py	`91.78% <0.00%> (+0.23%)`	⬆️
fury/convert.py	`80.00% <0.00%> (+6.66%)`	⬆️

fixed warring in test_utils.py

Nibba2018 · 2021-03-18T13:57:03Z

Hi @Shadow2121 , Thank you for creating this PR. Your changes look good to me. 👍

skoudoro · 2021-03-18T14:09:12Z

Hi @Shadow2121,

Thanks for the update, I will test the tutorial today and come back to you. Thanks

Shadow2121 · 2021-03-19T05:43:11Z

Hi @skoudoro

If we change the equation in tutorial then we also need to update orbit_points = np.zeros((1000, 3), dtype='f8') to orbit_points = np.zeros((2000, 3), dtype='f8') because it will not cover entire orbit.

And also need to update:

def timer_callback(_obj, _event):
    cnt = next(counter)
    showm.render()

    global mercury_track, venus_track, earth_track, mars_track, jupiter_track
    global saturn_track, uranus_track, neptune_track

    update_planet_position(r_mercury, mercury_actor, mercury_track, cnt)

    update_planet_position(r_venus, venus_actor, venus_track, cnt)

    update_planet_position(r_earth, earth_actor, earth_track, cnt)

    update_planet_position(r_mars, mars_actor, mars_track, cnt)

    update_planet_position(r_jupiter, jupiter_actor, jupiter_track, cnt)

    pos_saturn = update_planet_position(r_saturn, saturn_actor, saturn_track,
                                        cnt)
    saturn_rings_actor.SetPosition(pos_saturn[0], 0, pos_saturn[1])

    update_planet_position(r_uranus, uranus_actor, uranus_track, cnt)

    update_planet_position(r_neptune, neptune_actor, neptune_track, cnt)

    if cnt == 999:
        update_track(positions_mercury, mercury_track, mercury_orbit_actor)

        update_track(positions_venus, venus_track, venus_orbit_actor)

        update_track(positions_earth, earth_track, earth_orbit_actor)

        update_track(positions_mars, mars_track, mars_orbit_actor)

        update_track(positions_jupiter, jupiter_track, jupiter_orbit_actor)

        update_track(positions_saturn, saturn_track, saturn_orbit_actor)

        update_track(positions_uranus, uranus_track, uranus_orbit_actor)

        update_track(positions_neptune, neptune_track, neptune_orbit_actor)

    if cnt == 1500:
        showm.exit()

in this increase cnt for if cnt == 999: to if cnt == 1999: and if cnt == 1500: to if cnt == 2500:

Updated some values because we changed equation.

skoudoro

Hi @Shadow2121,

Thank you for the update. See below for some comments.

fury/tests/test_utils.py

docs/tutorials/01_introductory/viz_solar_system.py

As review suggested, changed some code.

Now, the path will update every call back.

Shadow2121 · 2021-03-23T05:53:06Z

Hello @skoudoro,

In this last commit I changed the call-back function, now it will update path every call-back. So there no need to update (show) path after certain time.

DEMO.mp4

skoudoro · 2021-03-23T13:51:26Z

Hi @Shadow2121,

Thank you for the update.

The creator of this tutorial chooses to update (show) path /orbit after a certain time for improving performance and saving memory at the beginning.
An alternative could be to draw the orbit from the beginning, using the line actor, and outside the callback function.

Could you look at this improvement?

Thank you

As suggested in review, now we are displaying the path/orbit before animation starts.

pep8speaks · 2021-03-23T16:09:01Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-15 03:54:54 UTC

Shadow2121 · 2021-03-24T04:14:58Z

Hi @skoudoro,

instead of using line actor I used update_track function to draw path/orbit.
How is it looking now?

skoudoro · 2021-03-24T23:02:15Z

Hi @Shadow2121,

You can use update_track AND line_actor. I recommend that you look deeper into this. When we zoom on the scene, we see that each track is composed of multiple points and there is space between them. The goal is to avoid that effect to improve the demo.

skoudoro

2 more comments to improve the tutorial.

you are close to being done! thank you!

skoudoro · 2021-03-24T23:06:45Z

docs/tutorials/01_introductory/viz_solar_system.py

+for i in range(2200):
+    calculate_path(r_mercury, mercury_track, i)
+    calculate_path(r_venus, venus_track, i)
+    calculate_path(r_earth, earth_track, i)
+    calculate_path(r_mars, mars_track, i)
+    calculate_path(r_jupiter, jupiter_track, i)
+    calculate_path(r_saturn, saturn_track, i)
+    calculate_path(r_uranus, uranus_track, i)
+    calculate_path(r_neptune, neptune_track, i)
+
+update_track(positions_mercury, mercury_track, mercury_orbit_actor)
+
+update_track(positions_venus, venus_track, venus_orbit_actor)
+
+update_track(positions_earth, earth_track, earth_orbit_actor)
+
+update_track(positions_mars, mars_track, mars_orbit_actor)
+
+update_track(positions_jupiter, jupiter_track, jupiter_orbit_actor)
+
+update_track(positions_saturn, saturn_track, saturn_orbit_actor)
+
+update_track(positions_uranus, uranus_track, uranus_orbit_actor)
+
+update_track(positions_neptune, neptune_track, neptune_orbit_actor)
+


this block can be reduced with 3-4 lines of code which avoids duplication.

use zip

call calculate_path and update_track only once in your loop

pass 2200 in your calculate_path

If i use line actor then there is no need for update_track,

skoudoro · 2021-03-24T23:07:30Z

docs/tutorials/01_introductory/viz_solar_system.py

@@ -183,7 +183,7 @@ def get_orbit_actor(orbit_points):
 # orbit actors into the scene. Also initialize the track variables for each
 # planet.

-orbit_points = np.zeros((2001, 3), dtype='f8')
+orbit_points = np.zeros((2200, 3), dtype='f8')


Can you replace f8 by float? Thank you.

Done, and sorry about that.

skoudoro

Hi @Shadow2121,

thanks for the update.

Some of my comments are not addressed
pep8 isssue are not addressed
the tutorial needs cleaning. many objects are not needed anymore like all orbital actors.

Some changes according reviews.

Shadow2121 · 2021-03-26T13:53:33Z

Hi @skoudoro,

Sorry about that, And

[ Done ] many objects are not needed anymore like all orbital actors.
[ Done ] pep8 isssue are not addressed
pass 2200 in your calculate_path -> For this I didn't get the point. If I pass 2200 to the function then it is not calculating for all points, it is doing calculation 2200 times for only one point.

Made some changes in documentation since changed so many code.

skoudoro

[ Done ] many objects are not needed anymore like all orbital actors.

There are still some useless text or functions like get_orbital_actor. Please, review completely the tutorial

[ Done ] pep8 isssue are not addressed

There is still some pep8 issue as you can see here.
Please, in general, try to pay more attention to details.

pass 2200 in your calculate_path -> For this I didn't get the point. If I pass 2200 to the function then it is not calculating for all points, it is doing calculation 2200 times for only one point.

Yes, this is why you will have to update calculate_path function

Shadow2121 · 2021-03-28T02:37:46Z

Oh, that's my bad I really thought that PEP 8 comment will pop at the bottom of our conversation, I will keep that in mind.

[ Done ] Review completely the tutorial
[ Done ] pep8 issue
[ Done ] update calculate_path function
And changed some comments(documentation).

Thank you

change according the review.

Shadow2121 · 2021-04-08T03:32:52Z

On my system(windows 10 64-bit) for different versions of python (3.7.0, 3.7.9, 3.9.1), I always get 1073741824.
Which OS are you using? @guaje

@Nibba2018 @skoudoro, do you guys still have the visual bug?

skoudoro · 2021-04-08T04:13:01Z

@Nibba2018 @skoudoro, do you guys still have the visual bug?

I do not have any the bug.

There is still some work to do in this tutorial.

But ok, we can merge this second version as it is. We will need to create a third version to reduce code duplication.
let me know if it is ok with you @guaje, @Nibba2018 and @nasimanousheh

Thanks @Shadow2121

Nibba2018 · 2021-04-08T12:34:37Z

@Shadow2121 @skoudoro @guaje

I dont have that visual bug either 👍

guaje · 2021-04-08T15:37:16Z

On my system(windows 10 64-bit) for different versions of python (3.7.0, 3.7.9, 3.9.1), I always get 1073741824.
Which OS are you using? @guaje

I did try in multiple Linux distributions (Arch-based and Debian-based) with different python versions (3.7.3, Anaconda 3.7.7, and 3.7.8).

But ok, we can merge this second version as it is. We will need to create a third version to reduce code duplication.
let me know if it is ok with you @guaje, @Nibba2018 and @nasimanousheh

I don't think adjusting the code duplication will take too much time/effort, so, why not do it now?

skoudoro · 2021-04-08T16:24:51Z

I don't think adjusting the code duplication will take too much time/effort, so, why not do it now?

Good question and I will let @Shadow2121 answer it

Shadow2121 · 2021-04-09T03:05:24Z

Ok I will do it.

But till now are there any major mistakes that I made that need to be changed.

Reduced some repetitive code. and some more changes like list name changed.

If there is any changes please let me know.

Shadow2121 · 2021-04-12T02:43:34Z

@skoudoro @guaje @Nibba2018 can you guys help? Are there any more changes that need to be done?

skoudoro

Hi @Shadow2121,

Thank you for the update. See below for some ideas to improve your version.
you are closer to the end of this PR!

docs/tutorials/01_introductory/viz_solar_system.py

Did some changes as per review.

Shadow2121 · 2021-04-13T06:04:38Z

@skoudoro,

Thank you very much for all your help. Done all the changes you mentioned.

It was raising the warning.

Shadow2121 · 2021-04-13T06:19:18Z

Changed data = np.array(data) to data = np.array(data, dtype=object).
Because It was raising the warning,

VisibleDeprecationWarning: Creating an ndarray from ragged nested sequences (which is a list-or-tuple of lists-or-tuples-or ndarrays with different lengths or shapes) is deprecated. If you meant to do this, you must specify 'dtype=object' when creating the ndarray
  data = np.array(data)

guaje

Thanks, @Shadow2121! The code looks way better. I just added a couple of suggestions to improve the readability of the tutorial.

docs/tutorials/01_introductory/viz_solar_system.py

Some more changes according to review.

Shadow2121 · 2021-04-14T04:44:33Z

@guaje, Thank you very much.

guaje

These new changes look good to me! Thanks, @Shadow2121

skoudoro

Hi @Shadow2121,

Everything looks almost good. See below my comments. I think they are the last ones. When it is fixed, I can approve and merge this PR.

Thanks again!

docs/tutorials/01_introductory/viz_solar_system.py

Final touch to tutorial.

skoudoro

Good job on the hard work. merging

Shadow2121 · 2021-04-15T14:04:18Z

Thank you :)

Fixed equation for calculating orbital period

5e74fcd

It is not much but I noticed it so I just changed it.

Update test_utils.py

76ccee7

fixed warring in test_utils.py

Shadow2121 changed the title ~~Fixed equation for calculating orbital period~~ Fixed warnnings in test_utils.py Mar 18, 2021

Shadow2121 changed the title ~~Fixed warnnings in test_utils.py~~ Fixed warnings in test_utils.py Mar 18, 2021

Nibba2018 approved these changes Mar 18, 2021

View reviewed changes

Update viz_solar_system.py

d101457

Updated some values because we changed equation.

skoudoro reviewed Mar 22, 2021

View reviewed changes

fury/tests/test_utils.py Outdated Show resolved Hide resolved

docs/tutorials/01_introductory/viz_solar_system.py Outdated Show resolved Hide resolved

docs/tutorials/01_introductory/viz_solar_system.py Outdated Show resolved Hide resolved

Shadow2121 added 2 commits March 23, 2021 09:56

Chenged test_utils.py and viz_solar_system.py

0537f68

As review suggested, changed some code.

Update viz_solar_system.py

ca36ea8

Now, the path will update every call back.

Update viz_solar_system.py

4e4b04a

As suggested in review, now we are displaying the path/orbit before animation starts.

Solved PEP 8 errors

0317ebd

skoudoro reviewed Mar 24, 2021

View reviewed changes

Update viz_solar_system.py

a08a148

skoudoro reviewed Mar 26, 2021

View reviewed changes

Update viz_solar_system.py

68622a2

Some changes according reviews.

update documentation

936e6cd

Made some changes in documentation since changed so many code.

skoudoro requested changes Mar 27, 2021

View reviewed changes

Shadow2121 added 2 commits March 28, 2021 08:40

Update viz_solar_system.py

06ef54c

change according the review.

solve PEP 8

b87bbdd

Shadow2121 added 3 commits April 9, 2021 10:32

Update viz_solar_system.py

955b4d9

Reduced some repetitive code. and some more changes like list name changed.

Some changes

755396a

If there is any changes please let me know.

fix PEP 8

87b7c8e

skoudoro reviewed Apr 12, 2021

View reviewed changes

Shadow2121 added 2 commits April 13, 2021 11:28

changes as per review

665ed36

Did some changes as per review.

Fix PEP 8

d0e71e2

Fix warning

6b77a67

It was raising the warning.

guaje requested changes Apr 13, 2021

View reviewed changes

Update viz_solar_system.py

7e11233

Some more changes according to review.

Shadow2121 requested review from skoudoro and guaje April 14, 2021 04:44

guaje approved these changes Apr 14, 2021

View reviewed changes

skoudoro reviewed Apr 14, 2021

View reviewed changes

Update viz_solar_system.py

2fb518f

Final touch to tutorial.

Shadow2121 requested a review from skoudoro April 15, 2021 03:55

skoudoro approved these changes Apr 15, 2021

View reviewed changes

skoudoro merged commit b7f09ab into fury-gl:master Apr 15, 2021

skoudoro added the type:Documentation label Apr 15, 2021

skoudoro added this to the v0.8.0 milestone Apr 15, 2021

Fixed warnings in test_utils.py #394

Fixed warnings in test_utils.py #394

Conversation

Shadow2121 commented Mar 17, 2021 • edited

codecov bot commented Mar 17, 2021 • edited

Codecov Report

Nibba2018 commented Mar 18, 2021

skoudoro commented Mar 18, 2021

Shadow2121 commented Mar 19, 2021

skoudoro left a comment

Choose a reason for hiding this comment

Shadow2121 commented Mar 23, 2021 • edited

skoudoro commented Mar 23, 2021

pep8speaks commented Mar 23, 2021 • edited

Comment last updated at 2021-04-15 03:54:54 UTC

Shadow2121 commented Mar 24, 2021

skoudoro commented Mar 24, 2021

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro Mar 24, 2021

Choose a reason for hiding this comment

Shadow2121 Mar 25, 2021 • edited

Choose a reason for hiding this comment

skoudoro Mar 24, 2021

Choose a reason for hiding this comment

Shadow2121 Mar 26, 2021

Choose a reason for hiding this comment

skoudoro left a comment

Choose a reason for hiding this comment

Shadow2121 commented Mar 26, 2021

skoudoro left a comment

Choose a reason for hiding this comment

Shadow2121 commented Mar 28, 2021 • edited

Shadow2121 commented Apr 8, 2021

skoudoro commented Apr 8, 2021

Nibba2018 commented Apr 8, 2021

guaje commented Apr 8, 2021

skoudoro commented Apr 8, 2021

Shadow2121 commented Apr 9, 2021

Shadow2121 commented Apr 12, 2021

skoudoro left a comment

Choose a reason for hiding this comment

Shadow2121 commented Apr 13, 2021

Shadow2121 commented Apr 13, 2021

guaje left a comment

Choose a reason for hiding this comment

Shadow2121 commented Apr 14, 2021

guaje left a comment

Choose a reason for hiding this comment

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro left a comment

Choose a reason for hiding this comment

Shadow2121 commented Apr 15, 2021

Shadow2121 commented Mar 17, 2021 •

edited

codecov bot commented Mar 17, 2021 •

edited

Shadow2121 commented Mar 23, 2021 •

edited

pep8speaks commented Mar 23, 2021 •

edited

Shadow2121 Mar 25, 2021 •

edited

Shadow2121 commented Mar 28, 2021 •

edited