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

FIX: missing color bar when direction flipped #341

Merged
merged 2 commits into from
Apr 14, 2017

Conversation

stevenjkern
Copy link
Contributor

Closes #311. The arange produced an empty array when the direction attribute is set to flipped since mapper.low_pos > mapper.high_pos. This PR tells arange to descend when using the flipped direction rather than ascend. Here is the example from #311 before:
image

and after:
image

@@ -140,7 +140,10 @@ def _draw_plot(self, gc, view_bounds=None, mode='normal'):

mapper = self.index_mapper

scrn_points = arange(mapper.low_pos, mapper.high_pos+1)
direction = 1 if self.direction == 'normal' else -1
Copy link
Contributor

@cfarrow cfarrow Feb 28, 2017

Choose a reason for hiding this comment

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

This does not do the right thing for flipped bounds. For example, for

low_pos = 1
high_pos = -1

this gives

array([1])

I suggest,

low = mapper.low_pos
high = mapper.high_pos
if self.direction == 'flipped':
    low, high = high, low
scrn_points = arange(low, high + 1)

and then delete lines 155-156, which would undo the bounds flip.

direction = 1 if self.direction == 'normal' else -1
scrn_points = arange(
mapper.low_pos, mapper.high_pos + 1, direction
)
Copy link
Member

Choose a reason for hiding this comment

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

What about the mapper.high_pos + 1 bit? Won't that need to change when the direction is flipped as well?

@stevenjkern
Copy link
Contributor Author

@jwiggins, I think that was covered by the change that @cfarrow suggested. Any further concerns or should I :shipit: ?

@jwiggins
Copy link
Member

Sorry, I ran into enthought/enable#266 when I was trying to test this. I've now tested it. LGTM!

@stevenjkern
Copy link
Contributor Author

Thanks, @cfarrow and @jwiggins! Merging now.

@stevenjkern stevenjkern merged commit 1e315e0 into master Apr 14, 2017
@stevenjkern stevenjkern deleted the fix/colorbar-reverse branch April 14, 2017 16:33
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.

3 participants