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

Use set_bounds rather than setting low and high individually for Zoom. #233

Merged
merged 3 commits into from Jan 22, 2016

Conversation

pberkes
Copy link
Contributor

@pberkes pberkes commented Nov 5, 2014

This reduces the number of event fired around drastically.

Compare this profile snapshot before the change:
screen shot 2014-11-05 at 11 23 20

and after:
screen shot 2014-11-05 at 11 23 30

Notice that the time per-call of _do_zoom is reduced from 0.00209 to 0.00122, but especially that _set_low_setting and _set_high_setting are called once per zoom_in / zoom_out, reducing the potential for trouble if anything depends on the bounds of the plot.

x_mapper.range.high = self.next[1]
y_mapper.range.low = self.next[2]
y_mapper.range.high = self.next[3]
x_mapper.range.set_bound(low=self.next[0], high=self.next[1])
Copy link
Member

Choose a reason for hiding this comment

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

should these be set_bounds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm yes! Sorry, I thought I had tested it but it looks like I don't know how to trigger this.

@itziakos
Copy link
Member

itziakos commented Nov 5, 2014

Are there any tests that cover this refactoring?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 135934e on zoom-use-set-bounds into 7d1ea31 on master.

@corranwebster
Copy link
Member

If you are doing this, it may be worth looking for places where the mapper high and low are set individually as well, eg. https://github.com/enthought/chaco/blob/master/chaco/data_view.py#L306

I had considered fixing those in an earlier PR, but it seemed tangential.

@pberkes
Copy link
Contributor Author

pberkes commented Nov 5, 2014

The example you give is changing a Mapper, which doesn't have set_bounds as far as I can tell

@corranwebster
Copy link
Member

It has a screen_bounds property which plays a similar role.

@pberkes
Copy link
Contributor Author

pberkes commented Nov 5, 2014

Got it

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 6987279 on zoom-use-set-bounds into 7d1ea31 on master.

corranwebster added a commit that referenced this pull request Jan 22, 2016
Use `set_bounds` rather than setting `low` and `high` individually for Zoom.
@corranwebster corranwebster merged commit f8d8781 into master Jan 22, 2016
@corranwebster corranwebster deleted the zoom-use-set-bounds branch January 22, 2016 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants