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 CursorTool issue with LogMapper #803

Merged
merged 8 commits into from
Jul 7, 2021

Conversation

aaronayres35
Copy link
Contributor

fixes #289
This PR pulls the content specific to #289 out from PR #726

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM.

Looks like this change was already made to CursorTool2D which is used for plots which inherit from Base2DPlot but the changes weren't applied earlier to CursorTool1D which is used for plots which inherit from BaseXYPlot.

chaco/tools/tests/test_cursor_tool.py Show resolved Hide resolved
@aaronayres35
Copy link
Contributor Author

Ah, similara to #804, this PR currently just makes a switch from one assumption to another. ie with the changes here, now the CursorTool will not work with LinearMappers. I will update the PR

@@ -122,7 +122,7 @@ def hittest(self, screen_pt, threshold=7.0, return_distance=False):
# screen_pt is one of the points in the lineplot
data_pt = (self.index.get_data()[ndx], self.value.get_data()[ndx])
if return_distance:
scrn_pt = self.map_screen(data_pt)
scrn_pt = self.map_screen(data_pt)[0]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this change is also made in #802

Both the changes to BaseXYPlot1.map_screen done there and LinearMapper.map_screen here can cause the return type to be an an array of shape (N,2).

@aaronayres35
Copy link
Contributor Author

Unfortunately a lot of these changes are some what related. The changes to LinearMapper made here could probably equally have gone in in #802

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

Still LGTM

@aaronayres35 aaronayres35 merged commit 787b7ba into master Jul 7, 2021
@aaronayres35 aaronayres35 deleted the fix/289-CursorTool-with-LogMapper branch July 7, 2021 13:51
@corranwebster corranwebster added the needs backport Needs to be backported to current maint/* label Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs backport Needs to be backported to current maint/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CursorTool does not work with LogMapper for both axes
3 participants