Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed some old tutorials. #591

Merged
merged 7 commits into from May 27, 2022
Merged

Conversation

m-agour
Copy link
Contributor

@m-agour m-agour commented May 27, 2022

Replaced deprecated method 'label' with 'vector_text'.
Also eliminated the "true division" runtime warning.

Replaced deprecated method 'label' with 'vector_text'.
Also eliminated the "true division" runtime warning.
@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #591 (ed4379a) into master (38e9c1f) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #591      +/-   ##
==========================================
- Coverage   87.96%   87.94%   -0.02%     
==========================================
  Files          62       62              
  Lines       13099    13099              
  Branches     1316     1316              
==========================================
- Hits        11522    11520       -2     
- Misses       1201     1202       +1     
- Partials      376      377       +1     
Impacted Files Coverage Δ
fury/fury/data/fetcher.py 71.03% <0.00%> (-1.38%) ⬇️

All actors' centers currently must be a ndarray.
Replaced deprecated method 'label' with 'vector_text'.
This line of code was triggering an error in "viz_sphere" and "viz_timer" tutorials.
Tests made me notice that radii can be either a float or a ndarray.
Copy link
Member

@xtanion xtanion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @m-agour for this PR.
Please take a look at my comment below. Thanks

fury/actor.py Outdated
if type(radii) in [float, int]:
scales = np.array([1, 1, 1]) * radii
else:
scales = np.array([1, 1, 1]) * radii[:, None]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works perfectly fine @m-agour ,
You may also use scales = np.array(radii) as repeat_primitive handles the reshaping of the scales array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @xtanion,
Thank you for reviewing my PR.
The compact solution you mentioned works just fine. Thanks for mentioning it.

Copy link
Contributor

@ganimtron-10 ganimtron-10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @m-agour ,
All your changes are perfectly fine!!

There is just an issue with the sphere texture in viz_earth_animation which is inverted horizontally as shown below!!

earth_animation_issue
earth_animation_issue_2

@m-agour
Copy link
Contributor Author

m-agour commented May 27, 2022

Hello @ganimtron-10,
Thank you for pointing out this issue. I'm looking into it.

No need to do any checks of conversions since it's all done inside the repeat_primitive function.

Co-Authored-By: Shivam Anand <74976752+xtanion@users.noreply.github.com>
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this @m-agour,

Overall, looks good to me. Good catch for the reverted earth @ganimtron-10! I think, this should be fixed in a new PR otherwise the title will not correspond to the change.

So, I will go ahead and merge it. Thanks all

@skoudoro skoudoro merged commit b9b29eb into fury-gl:master May 27, 2022
@m-agour m-agour changed the title Replaced a deprecated method. Replaced deprecated label and fixed some old tutorials. Oct 8, 2022
@m-agour m-agour changed the title Replaced deprecated label and fixed some old tutorials. Fixed some old tutorials. Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants