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

Turn off adaptive step if "step" option is given #425

Merged
merged 3 commits into from
Jun 22, 2022
Merged

Turn off adaptive step if "step" option is given #425

merged 3 commits into from
Jun 22, 2022

Conversation

hanjinliu
Copy link
Contributor

This PR changes the default behavior of step and adaptive_step.

I like the idea of using adaptive step by default but it should be turned off if step is explicitly given (also in terms of backward compatibility).
It is not straightforward that the step option is ignored unless "adaptive_step": False is specified at the same time.

@magicgui(x={"step": 0.1})  # ignored
def func(x: float): ...

@magicgui(x={"step": 0.1, "adaptive_step": False})  # OK
def func(x: float): ...

To do this, I corresponded step=None to adaptive step mode, and accordingly, the default value of step is changed from 1 to UNSET and step returns None if the widget is in adaptive step mode.
This change might violate compatibility if one checks the value like widget.step == 1 but it seems very rare.

@tlambert03 tlambert03 requested a review from Czaki June 16, 2022 16:32
@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Merging #425 (0528bf5) into main (0bfb7bd) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   88.94%   88.99%   +0.05%     
==========================================
  Files          30       30              
  Lines        3926     3935       +9     
==========================================
+ Hits         3492     3502      +10     
+ Misses        434      433       -1     
Impacted Files Coverage Δ
magicgui/backends/_qtpy/widgets.py 87.00% <100.00%> (+0.11%) ⬆️
magicgui/widgets/_bases/ranged_widget.py 87.93% <100.00%> (+1.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bfb7bd...0528bf5. Read the comment docs.

@@ -29,8 +29,7 @@ def __init__(
self,
min: Union[float, _Unset] = UNSET,
max: Union[float, _Unset] = UNSET,
step: float = 1,
adaptive_step: bool = True,
step: Union[float, _Unset, None] = UNSET,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you motivate me to why you add None and _Unset here? One of them should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because min and max are also initialized with UNSET... I guess UNSET is for value only (because it might be set to None intentionally) so should we just change the defaults of min, max and step to None?

@Czaki
Copy link
Contributor

Czaki commented Jun 16, 2022

I apologize for missing backward compatibility in my PR.

Copy link
Member

@tlambert03 tlambert03 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 @hanjinliu

@tlambert03 tlambert03 merged commit bbf728f into pyapp-kit:main Jun 22, 2022
@tlambert03 tlambert03 added the bug Something isn't working label Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants