-
Notifications
You must be signed in to change notification settings - Fork 182
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
Surface actor colormap fix #430
Surface actor colormap fix #430
Conversation
Codecov Report
@@ Coverage Diff @@
## master #430 +/- ##
=======================================
Coverage 88.87% 88.88%
=======================================
Files 23 23
Lines 5393 5396 +3
Branches 696 697 +1
=======================================
+ Hits 4793 4796 +3
Misses 415 415
Partials 185 185
|
@amitchaudhari9121 Thanks for the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @amitchaudhari9121,
Thank you for this fix. As usual, it needs tests to avoid reproducing this issue.
Furthermore, we need to check what is the rules. Are the other actors expecting 0-1 or 0-255. we should make sure that is uniform everywhere.
I've skimmed
I'm not sure on how do i go about testing it. Do we need to test it? |
I think you could raise an error when the user inputs a color that's out of bounds. @skoudoro will be able to guide better. |
We might let pass this PR and create a specific PR for all actors to raise an error if a color is out of bounds. Still thinking about it 🤔 ... |
Ok 👌 |
@amitchaudhari9121, @skoudoro, and @SunTzunami. This is blocking #362. Is any of you working on the "check out of bounds" PR or what are the steps to merge/close this and #362? |
@guaje once this is done, #362 will work as intended and I'll update it to be merged. Unsure about -
@skoudoro and @amitchaudhari9121 |
@guaje I can't find the time to work for fury for the moment, I'm busy with my intern at Linux foundation, As far as i remember (its been weeks since those commits), going out of bounds just messes up the colormap displayed. Personally, I don't think we need to add tests for it, it's mentioned clearly in the doc for that function, and any wrong input does not crash anything. We could merge this as is, and not have any problems. @skoudoro @SunTzunami your thoughts? |
Hi All, This PR can be merged so I will go ahead. However, we need to clarify the rules concerning the color ([0-1], vs [0-255]). Let's create an issue to remember that we need to handle the color in all actors. I suppose, there is a function for that in utils.py or we need to create it. |
Thanks @amitchaudhari9121 for this. Merging! |
Hi!
This is a fix for issue #429 .
@SunTzunami and I hopped on a call and fixed it together.
Earlier
Now
I think, this is exactly how It should look...
I've also changed the documentation to include the range of the Individual color values.
In case someone else is confused due to this.