-
Notifications
You must be signed in to change notification settings - Fork 929
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
feat: Implement Slider class for JupyterViz #1972
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1972 +/- ##
==========================================
- Coverage 79.27% 79.00% -0.28%
==========================================
Files 16 17 +1
Lines 1298 1324 +26
Branches 291 295 +4
==========================================
+ Hits 1029 1046 +17
- Misses 230 237 +7
- Partials 39 41 +2 ☔ View full report in Codecov by Sentry. |
I really like this API. |
485365c
to
5f53869
Compare
Added test. This PR is ready for review. |
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.
Really like the API, this is good stuff.
Do we want to drop the explicit distinction between float and int? Is that implied with the values (mainly step) that you input now?
Will try to do a full review later today.
I am fine with dropping the distinction, but I can see arguments to have explicit int and float sliders. If desired, one possibility would be to just add a dtype kwarg to optionally control this behavior. If dtype is none, you derive the dtype from the arguments. |
5f53869
to
a1ddac1
Compare
Added an optional dtype argument. |
This provides API parity with the previous Tornado viz.
a1ddac1
to
c290058
Compare
@ankitk50 since you’re also working of visualization, what’s your take? |
I would agree with being explicit with float/int data type. This would also later cater to the possibility of a custom slider with strings |
With the optional dtype argument, it should be quite easy to expand to categorical sliders with string labels later down the line. I can't fully judge the code because I am unfamiliar with solara. But from an API and extensibility point of view, this looks all fine to me. |
Really like how much more concise this is to configure. |
Should it have an |
That would need an extra button "reset to initial value" in order to be usable, and would be out of the scope of this PR. This PR wraps the same number of params in Solara's slider. |
I agree with @rth. Let's merge this. |
This provides API parity with the previous Tornado viz. Supersedes #1945.
For the Schelling experimental example, from
to
I haven't added test because still waiting for agreement. But I have tested this manually on the Schelling experimental example.