Skip to content

FIX/ENH: Introduce a monolithic legend handler for Line2D #20699

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

Merged
merged 7 commits into from
Aug 14, 2021

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 20, 2021

PR Summary

This is a rebase of #11358 (fixing #2035, #11357, #18391, #20552) on top of #20670 (to address #11358 (comment)) and with an additional commit on top to address other comments.

I decided to keep both legend handlers public for now (based on comments in the original thread); we can always decide to deprecate the old handler later.

PR Checklist

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

... except for markevery=<float> or (<float>, <float>) which is a
spec relative to the axes size (keeping handling of that specific case
into _mark_every_path).

@anntzer anntzer force-pushed the monolithic-line2d-legend-handler branch from f28247a to 471ae33 Compare July 20, 2021 19:21
@jklymak jklymak marked this pull request as draft July 20, 2021 20:32
@anntzer anntzer marked this pull request as ready for review July 21, 2021 19:32
@jklymak
Copy link
Member

jklymak commented Jul 28, 2021

Without digging through the discussion, can you explain this deprecation path? I'd have thought that changing the behaviour of the returned object was worse than explicitly returning a different class.

@anntzer anntzer force-pushed the monolithic-line2d-legend-handler branch from 471ae33 to 3539f18 Compare July 28, 2021 14:12
@anntzer
Copy link
Contributor Author

anntzer commented Jul 28, 2021

I don't have a very strong opinion here, but do you have another proposal? (Preferably without rehashing the whole discussion at #11358...)
Note that I believe having a noisy deprecation (to force people to explicitly opt-in the new handler, perhaps via a rcparam) is not realistic because it will impact a huge number of people using legends even though only a tiny minority (those accessing the underlying artists to modify it) will actually be affected by the API change; moreover, in all likelihood, they were actually relying on private attributes anyways (#11358 (comment), #20552).

@jklymak
Copy link
Member

jklymak commented Jul 28, 2021

I thought the alternative would be to return a new class (ideally a subclass of the old one). It would only break people explicitly relying on the different behaviour. To get the old behaviour they would get the old class back.

@jklymak
Copy link
Member

jklymak commented Jul 28, 2021

Btw I agree that it's not worth a noisy deprecation cycle.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 28, 2021

Do you mean calling the new handler e.g. HandlerLine2DMonolithic (and registering that one by default) and keeping the old one as HandleLine2D but not registering it? Sure, I don't have a strong preference here.

@jklymak
Copy link
Member

jklymak commented Jul 28, 2021

I don't know enough of the details to have a strong preference either

@anntzer
Copy link
Contributor Author

anntzer commented Jul 28, 2021

OK, I'll let other reviewers chime in then.

@timhoffm
Copy link
Member

timhoffm commented Jul 28, 2021

So the main difference is returning [legline, legline_marker] (old) vs [legline2d] (new) in create_artists(). A real deprecation with warning might be possibly. We'd need to return [obj1, obj2], where obj1 and obj2 fulfill the old API and obj1 fulfills the new API. That could possibly be resolved by returning Line2DHandleList(legline2d), where Line2DHandleList is like [legline2d, legline2d] but warns whenever the second element is accessed. I don't think we need to proxy the line objects themselves because legline2d is a superset of legline functionality (The only breakage would be that formerly you could have done legline.set_marker() which now would have a different effect, but that's quite artificial).

class Line2DHandleList(Sequence):
   def __init__(self, legline):
       self._legline = legline

   def __len__(self):
       return 2

   def __getitem__(self, index):
       if index != 0:
           _api.warn_deprecated(...)
       return [self.legline, self.legline][index]

Off the top of my head, that should cover all realistic use cases of the present API.

Do you mean calling the new handler e.g. HandlerLine2DMonolithic (and registering that one by default) and keeping the old one as HandleLine2D but not registering it?

I suppose nobody is using these classes by name. So, which one is the HandlerLine2D is not particularly important. Therefore I vote to switch the naming. Having HandlerLine2D in the long run is better.

If people need to keep compatibility with older versions, they can do:

from matplotlib.lines import Line2D
try:
    from matplotlib.legend_handler import HandlerLine2DCompound
    mlegend.Legend.update_default_handler_map({Line2D: HandlerLine2DCompound()})
except ImportError:
    pass

which should be document in the API change note.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 29, 2021

Sure, it's a bit wordy because the normal machinery shouldn't trigger the deprecation, but it's done now.

@anntzer anntzer force-pushed the monolithic-line2d-legend-handler branch from c0e1c1b to f86bfa1 Compare July 29, 2021 09:39
@anntzer anntzer force-pushed the monolithic-line2d-legend-handler branch from f86bfa1 to a25fd75 Compare July 29, 2021 09:48
@tacaswell tacaswell modified the milestone: v3.6.0 Aug 14, 2021
@tacaswell tacaswell added this to the v3.5.0 milestone Aug 14, 2021
@tacaswell tacaswell merged commit 1761822 into matplotlib:master Aug 14, 2021
ianhi added a commit to mpl-extensions/mpl-point-clicker that referenced this pull request Aug 15, 2021
@anntzer anntzer deleted the monolithic-line2d-legend-handler branch August 16, 2021 05:34
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.

5 participants