Skip to content

Added a new public API update_range to Slider widget. It helps to change the valmin and valmax of the Slider #20579

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

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

BSNayal
Copy link

@BSNayal BSNayal commented Jul 5, 2021

PR Summary

This PR creates a new public API for slider . It is called the udpate_range(). So when a slider has been created and later on we want to update its range we can use this function

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

Copy link

@github-actions github-actions bot 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 opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@jklymak jklymak added this to the v3.5.0 milestone Jul 5, 2021
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and welcome to Matplotlib!

@jklymak jklymak marked this pull request as draft July 6, 2021 13:43
@timhoffm
Copy link
Member

timhoffm commented Jul 6, 2021

@BSNayal In general, you should not merge master into your feature branch. Instead rebase your branch on master if needed (i.e. if you need some new code from master or need to resolve a conflict). Otherwise, it's ok if the feature branch is behind master.

@BSNayal
Copy link
Author

BSNayal commented Jul 6, 2021

@BSNayal In general, you should not merge master into your feature branch. Instead rebase your branch on master if needed (i.e. if you need some new code from master or need to resolve a conflict). Otherwise, it's ok if the feature branch is behind master.

Sure.
Actually in our normal workflow, we usually first update our feature branch with the latest changes from the master branch so that our changes are at the top of the latest changes and then pushed.

@BSNayal
Copy link
Author

BSNayal commented Jul 6, 2021

I am currently working on windows environment.
Do I need to test these on other environments too ? (Linux , Mac jobs keep failing)

@jklymak
Copy link
Member

jklymak commented Jul 6, 2021

All the tests have to pass, but hopefully you can fix them without access to other operating systems.

@ianhi ianhi self-requested a review July 6, 2021 18:36
@ianhi
Copy link
Contributor

ianhi commented Jul 6, 2021

One thing to consider is that #19265 will have an effect on this, or vice versa.

@@ -529,6 +529,25 @@ def on_changed(self, func):
"""
return self._observers.connect('changed', lambda val: func(val))

def set_limits(self, vmin=None, vmax=None):
"""Update the range of the slider"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Update the range of the slider"""
"""Update the limits of the slider."""

1. Add a set_limits function to the SliderBase class
@timhoffm
Copy link
Member

Please rebase on master to fix the doc build.

@BSNayal
Copy link
Author

BSNayal commented Jul 16, 2021

can we create timer based image comparison test cases ? My scenario : the plot is shown with the slider, after 1 sec I change the val of the slider and after 2 sec I change the limits of the slider. So once the limits are changed after that the snapshot should be taken , but in my case when the plot is shown the image is captured at that moment

@QuLogic
Copy link
Member

QuLogic commented Jul 20, 2021

The plots are never shown in tests; they are not interactive, ergo, there's no need to wait to see what changes. What's captured is always the last appearance of the figure.

@@ -306,6 +306,19 @@ def reset(self):
if self.val != self.valinit:
self.set_val(self.valinit)

def set_limits(self, vmin=None, vmax=None):
Copy link
Member

Choose a reason for hiding this comment

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

To match __init__, these should probably be named:

Suggested change
def set_limits(self, vmin=None, vmax=None):
def set_limits(self, valmin=None, valmax=None):

@@ -583,6 +596,18 @@ def on_changed(self, func):
"""
return self._observers.connect('changed', lambda val: func(val))

def set_limits(self, vmin=None, vmax=None):
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above about argument names.

self.hline.set_ydata(self.valinit)
else:
self.vline.set_xdata(self.valinit)


class RangeSlider(SliderBase):
Copy link
Member

Choose a reason for hiding this comment

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

Does RangeSlider also need some custom set_limits, or is the base class implementation enough?

@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 18, 2021
@tacaswell
Copy link
Member

@BSNayal Could you use git rebase -i to simplify the history of this PR?

@ianhi
Copy link
Contributor

ianhi commented Oct 26, 2021

Hi @BSNayal what are your plans for this PR? If you no longer have time for it then I'd be happy to pick it up and finish the good work you started.

@QuLogic QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022
@tacaswell
Copy link
Member

@BSNayal Are you still interested in working on this?

I'm going to push this to 3.8.

@tacaswell tacaswell closed this Dec 16, 2022
@tacaswell tacaswell reopened this Dec 16, 2022
@tacaswell
Copy link
Member

(sorry miss-clicked, did not mean to close)

@tacaswell tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 16, 2022
@ksunden ksunden modified the milestones: v3.8.0, future releases Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

7 participants