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

Fixing ZeroDivisionError thrown by UI sliders when the value_range is zero (0) #645

Merged
merged 5 commits into from
Aug 10, 2022

Conversation

ganimtron-10
Copy link
Contributor

Currently, the UI sliders throw ZeroDivisionError whenever the value_range is zero ie. min_value and max_value are the same.
Adding checks to avoid these errors and setting the handle at the start.

@ganimtron-10 ganimtron-10 requested review from m-agour and xtanion July 28, 2022 17:29
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #645 (b5835d0) into master (bca7b33) will decrease coverage by 1.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #645      +/-   ##
==========================================
- Coverage   52.04%   51.02%   -1.03%     
==========================================
  Files         108      108              
  Lines       23812    23894      +82     
  Branches     2631     2631              
==========================================
- Hits        12394    12192     -202     
- Misses      11013    11293     +280     
- Partials      405      409       +4     
Impacted Files Coverage Δ
fury/ui/elements.py 0.00% <0.00%> (ø)
fury/ui/tests/test_containers.py 0.00% <0.00%> (ø)
fury/ui/tests/test_elements.py 0.00% <0.00%> (ø)
fury/fury/tests/test_window.py 50.94% <0.00%> (-35.43%) ⬇️
fury/fury/ui/tests/test_containers.py 64.76% <0.00%> (-28.96%) ⬇️
fury/fury/window.py 72.85% <0.00%> (-10.41%) ⬇️
fury/fury/shaders/base.py 84.84% <0.00%> (-8.09%) ⬇️
fury/fury/ui/containers.py 82.92% <0.00%> (-0.89%) ⬇️
fury/fury/data/fetcher.py 67.81% <0.00%> (-0.43%) ⬇️
fury/fury/actor.py 88.87% <0.00%> (-0.19%) ⬇️
... and 6 more

@m-agour
Copy link
Contributor

m-agour commented Jul 28, 2022

Hello @ganimtron-10,
Thanks for making this PR, it's really important. Also, thanks for adding the unit test.

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.

Hi @ganimtron-10, I tried this with this animation example and the Fox model. It was causing ZeroDivisionError previously. It's working fine now 👍🏽.

I still need to check this with different models, I'll let you know if I find anything.

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.

hi @ganimtron-10,

The fix looks good to me. however, the tests are weird.... Can you update your tests? thanks

fury/ui/tests/test_elements.py Outdated Show resolved Hide resolved
@m-agour
Copy link
Contributor

m-agour commented Aug 4, 2022

Hi @ganimtron-10 ,
Thank you for updating the tests. Can you also test for other range cases such as the following for each slider?

    with npt.assert_no_warnings():
        line_slider = ui.LineSlider2D(min_value=0, max_value=0,
                                      center=(200, 450))

        assert_equal(line_slider.value, 0)
        assert_equal(line_slider.min_value, 0)
        assert_equal(line_slider.max_value, 0)
        line_slider.value = 100
        assert_equal(line_slider.value, 0)
        line_slider.value = -100
        assert_equal(line_slider.value, 0)

        line_slider = ui.LineSlider2D(min_value=0, max_value=100,
                                      center=(200, 450))
        line_slider.value = 105
        assert_equal(line_slider.value, 100)
        line_slider.value = -100
        assert_equal(line_slider.value, 0)

@ganimtron-10
Copy link
Contributor Author

Thank You @m-agour for the suggestion.
I have updated the tests!
PTAL.
Thanks!

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.

LGTM, thanks @ganimtron-10 and @m-agour ! merging

@skoudoro skoudoro merged commit 3830c31 into fury-gl:master Aug 10, 2022
@ganimtron-10 ganimtron-10 deleted the slider-error branch August 10, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants