Skip to content

Conversation

xordux
Copy link

@xordux xordux commented Sep 15, 2020

PR Summary

Added a change in lib/matplotlib/backend_tools.py to address #18151

The values of navigate_mode are set to either 'PAN', 'ZOOM', or None. This is because the comment in _AxesBase.get_navigate_mode() says:

Get the navigation toolbar button status: 'PAN', 'ZOOM', or None

PR Checklist

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

This change sets navigate_mode in axes.
@xordux
Copy link
Author

xordux commented Sep 15, 2020

There is 1 pytest case which was already failing(locally) and is still failing(locally): test_get_fontconfig_fonts

@dopplershift
Copy link
Contributor

It would be great to add some kind of automated test for this behavior, maybe in test_backend_tools.py?

@xordux
Copy link
Author

xordux commented Sep 16, 2020

It would be great to add some kind of automated test for this behavior, maybe in test_backend_tools.py?

yes, I have a code which I used to test this. Think that can be modified into a test case. Should I add that code in this PR or a different one?

@xordux xordux force-pushed the navigate_mode_18151 branch from 74b3696 to 882f485 Compare September 16, 2020 05:30
@tacaswell
Copy link
Member

This PR please.

Thank you for working on this :)

@tacaswell tacaswell added this to the v3.4.0 milestone Sep 16, 2020
@xordux xordux force-pushed the navigate_mode_18151 branch from 3c01c15 to fdb21b7 Compare September 16, 2020 07:04
@xordux xordux requested a review from QuLogic September 16, 2020 13:39
@xordux xordux force-pushed the navigate_mode_18151 branch from fdb21b7 to 92f6a11 Compare September 23, 2020 09:14
@xordux xordux requested a review from QuLogic September 23, 2020 09:15
@xordux
Copy link
Author

xordux commented Sep 23, 2020

I am getting this warning ToolManager does not control tool rubberband from this line ax.figure.canvas.manager.toolmanager.trigger_tool('pan') in test case. I was not sure if this is a issue, so I suppressed it. Kindly let me know if this is an issue and needs to be fixed first.

@tacaswell
Copy link
Member

That last warning seems like something you should chase down as that is coming from

def get_tool(self, name, warn=True):
"""
Return the tool object with the given name.
For convenience, this passes tool objects through.
Parameters
----------
name : str or `.ToolBase`
Name of the tool, or the tool itself.
warn : bool, default: True
Whether a warning should be emitted it no tool with the given name
exists.
Returns
-------
`.ToolBase` or None
The tool or None if no tool with the given name exists.
"""
if isinstance(name, tools.ToolBase) and name.name in self._tools:
return name
if name not in self._tools:
if warn:
cbook._warn_external("ToolManager does not control tool "
"%s" % name)
return None
return self._tools[name]

@xordux xordux requested a review from dopplershift September 24, 2020 05:45
Comment on lines 181 to 184
assert(len(rec) == 4)
for r in rec:
assert("The new Tool classes introduced in v1.5 are experimental"
in str(r.message))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this needs to be asserted as part of this test specifically. However, if this is to ensure that other unexpected warnings are not raised, then it can be done by specifying the match argument in the pytest.warns call.

Copy link
Member

Choose a reason for hiding this comment

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

@QuLogic do you want this to be resolved before merging?

Copy link
Member

Choose a reason for hiding this comment

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

I was hoping for a reply from @xordux about the intention at least.

Copy link
Author

Choose a reason for hiding this comment

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

Oops I thought it's an advice that pytest.warns can also be used for the same. @QuLogic Yes, while asserting warning messages my intentions were to ensure that any other unexpected warnings are not raised.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should switch to pytest.warns then.

Copy link
Author

@xordux xordux Oct 14, 2020

Choose a reason for hiding this comment

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

@QuLogic I have added the match argument in pytest.warns but now some other test cases are failing which don't seem to be related to my changes.

Edit: I also synced my forked code with the https://github.com/matplotlib/matplotlib.git

@jklymak
Copy link
Member

jklymak commented Oct 10, 2020

@dopplershift or @fariza do either of you have time to look at this?

@jklymak
Copy link
Member

jklymak commented Oct 14, 2020

The failing mac test seems to be common across other recent PRs and possibly due to some imagmagick changes

The failing tk test on Windows, I'm not sure about. Other PRs are not similarly failing?

@xordux
Copy link
Author

xordux commented Oct 14, 2020

closing to re-test the checks.

@xordux xordux closed this Oct 14, 2020
@xordux xordux reopened this Oct 14, 2020
@xordux xordux closed this Oct 14, 2020
@xordux xordux reopened this Oct 14, 2020
@xordux
Copy link
Author

xordux commented Oct 14, 2020

The failing mac test seems to be common across other recent PRs and possibly due to some imagmagick changes

The failing tk test on Windows, I'm not sure about. Other PRs are not similarly failing?

@jklymak The failing tk test on Windows passed when the checks were re-rerun. Only mac test are failing due to a issue in latest version of imagemagick (issue raised here)

@jklymak jklymak merged commit 9d06b1c into matplotlib:master Oct 14, 2020
@jklymak
Copy link
Member

jklymak commented Oct 14, 2020

Thanks @xordux, and @QuLogic and @fariza for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants