Fix progress bar float input bug #7953
Conversation
| def _check_float_between(value: float, low: float = 0.0, high: float = 1.0) -> bool: | ||
| """ | ||
| Checks given value is 'between' the bounds of [low, high], | ||
| considering close values around bounds are acceptable input |
There was a problem hiding this comment.
I think it would be great if you extended the comment slightly explaining why we want to consider close values around bounds, e.g. ... acceptable input, for example in order to account for values being slightly off due to floating point operations.. What do you think?
There was a problem hiding this comment.
@raethlein I agree. Thank you for your comment. I have extended the docstring in commit 98ff93b4b3af986ef4fde89b8689a22c81dc7a09.
lib/streamlit/elements/progress.py
Outdated
| if ( | ||
| (low <= value <= high) | ||
| or math.isclose(value, low, rel_tol=1e-9, abs_tol=1e-9) | ||
| or math.isclose(value, high, rel_tol=1e-9, abs_tol=1e-9) | ||
| ): | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
nit: this could be cleaned up a tiny bit by just returning the conditional in the if rather than returning True or False based on it
return (
(low <= value <= high)
or math.isclose(value, low, rel_tol=1e-9, abs_tol=1e-9)
or math.isclose(value, high, rel_tol=1e-9, abs_tol=1e-9)
)There was a problem hiding this comment.
@vdonato I thought at first since the if condition is quite long it would hurt readability to simply return the expression, but now that I see your suggestion it looks cleaner. Thank you for the comment. I have fixed it in commit 98ff93b4b3af986ef4fde89b8689a22c81dc7a09.
There was a problem hiding this comment.
One tiny nit (and also seconding @raethlein's comment), but otherwise this LGTM. Thanks for the contribution @notiona!
|
Thank you for your valuable comments guys! I will address them and let you guys know when I am done. |
|
I have addressed all your comments. Feel free to provide any other feedback. Thank you for the great work! @LukasMasuch @sfc-gh-lmasuch @raethlein @vdonato |
|
Thanks for the update :) |
* Fix progress bar float input bug * add docstring, simplify fix st.progress bar bug fix
Describe your changes
This PR solves the wrongly raised
streamlit.errors.StreamlitAPIExceptionwhen giving a value slightly greater than 1.0 forst.progress_bar. The issue is stated here.Even after mitigating the issue directly, the PR also handles cases where given value for
st.progress_baris slightly below 0.0, which should also cause a similar bug.Instead of simply rounding the given float value, I have decided to use
math.isclose()function as it provides both relative tolerance and absolute tolerance capabilities. Also, performance-wise, by checkingmath.isclose()in the same if condition as the previous condition, we would have no excess computation for normal values due to short circuit evaluation.Additionally if required, this helper function
_check_float_betweencan be reused in other parts of streamlit where flexible float comparison is necessary. Thus the low and high bounds are configurable as keyword arguments.GitHub Issue Link (if applicable)
Closes #5517
Testing Plan
lib/tests/streamlit/elements/progress_test.pychecking for values that are off the bound of(low <= value <= high), yet still should be considered valid input.Please feel free to give any ideas on improvements or suggestions. 馃槃
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.