Skip to content

Correctly set formatters and locators on removed shared axis#14997

Merged
tacaswell merged 1 commit intomatplotlib:masterfrom
dstansby:fmt-loc-setting
Aug 19, 2019
Merged

Correctly set formatters and locators on removed shared axis#14997
tacaswell merged 1 commit intomatplotlib:masterfrom
dstansby:fmt-loc-setting

Conversation

@dstansby
Copy link
Copy Markdown
Member

@dstansby dstansby commented Aug 6, 2019

Fixes #14911

The issue was that a shared axis might never have it's Formatter/Locator set, which is fine when it is shared, but when the axis storing the Formatter/Locator it relies on is removed, the previous shared axis needs to have the Formatter/Locators set.

Also fixed typo _reset_axis_form > _reset_axis_from.

I've tried a few things, but cannot come up with a more elegant solution than this; if anyone has any ideas suggestions welcome.

@dstansby dstansby added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 6, 2019
@dstansby dstansby added this to the v3.2.0 milestone Aug 6, 2019
Comment thread lib/matplotlib/figure.py Outdated
@jklymak
Copy link
Copy Markdown
Member

jklymak commented Aug 6, 2019

Ok so I think this is a case of sharex=True meaning too many things. Are we garaunteeibg that sharing the axis means we are sharing the locators and formatters? That seems really hard to enforce and when you toss units into the mess, it becomes almost intractable. I agree that just removing an axis shouldn’t cause a crash, but I think the fundamental issue is that the locators and formatters were linked in the first place.

@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Aug 6, 2019

Are we garaunteeibg that sharing the axis means we are sharing the locators and formatters?

At least IIRC @efiring has a fairly strong opinion that we do...

Comment thread lib/matplotlib/figure.py
axis.set_major_formatter(majfmt)
if isDefault:
majfmt.axis.isDefault_majfmt = True

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't make any sense to me. You get the formatter for axis and then you set the formatter for axis to that formatter, and then you set the isDefault to the value axis had before. I don't see how this code is doing anything.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think the confusion here stems from the mappings of formatters to axis.

  • Axis > Formatter is a one to one mapping
  • Formatter > Axis is a one to many mapping

So a single Formatter is linked to a single Axis, but many other Axis can make use of that Formatter. I agree this is a nightmare to understand, but without re-doing the internals of how this works it is how it is.

So this code is getting the formatter that axis is using, (that it might be borrowing) and then explicitly associating that formatter with axis (so it is no longer borrowing it).

@dstansby
Copy link
Copy Markdown
Member Author

I appreciate this is messy code, but it is neccesitated by the weird way that Formatter/Locator and Axis interact. I think the options are

  1. Merge this PR as a fix (+/- small changes)
  2. Change the way Formatter and Locator work with shared Axis

I think 2. is a bit much to do for 3.2, but maybe should be looked at for 3.3?

@jklymak
Copy link
Copy Markdown
Member

jklymak commented Aug 10, 2019

Be good to rebase to double check that docs build, but I don't think that has anything to do w/ this PR.

Comment thread lib/matplotlib/tests/test_figure.py
@dstansby dstansby changed the title Correct set formatters and locators on removed shared axis Correctly set formatters and locators on removed shared axis Aug 13, 2019
@tacaswell tacaswell merged commit e04ebd2 into matplotlib:master Aug 19, 2019
@dstansby dstansby deleted the fmt-loc-setting branch August 21, 2019 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a shared axes via ax.remove() leads to an error.

6 participants