Skip to content

Re-rename builtin seaborn styles to not include a dot. #23674

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 1 commit into from
Aug 26, 2022

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Aug 19, 2022

... as we may want to use dots later to support third-party styles, e.g.
foo.bar could be used to mean foo/bar.mplstyle where foo is a
regularly installed python package.
(This is an idea I had after rethinking about externally-distributable styles.)

Marking as RC as the original renaming also came in in mpl3.6, and we don't want to rename twice...
@tacaswell @timhoffm @QuLogic were involved in discussing the name in the original PR (#22317), so pinging them again here.

PR Summary

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • 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).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@anntzer anntzer added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: styles labels Aug 19, 2022
@anntzer anntzer added this to the v3.6.0 milestone Aug 19, 2022
Copy link
Member

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

Looks good! I'm not confident enough that there is a spurious failure, although I think so, but feel free to self-merge if you are.

@timhoffm
Copy link
Member

timhoffm commented Aug 19, 2022

I'm always slightly annoyed when I have to write a version number but the format does not support dots. 08 is less readable than 0.8.

Is it likely that we'll want the dot as a namespace separator later on and can't do with another good choice? For this the question is important, where we expect to use the name and whether that has restrictions.

I think that : or / may be more appropriate namespace separators.

Where might w want to use this?

  • Strings like in plt.style.use(name): "seaborn-colorblind-0.8", "seaborn:colorblind-0.8", "seaborn/colorblind-0.8", or seaborn.colorblind08.
  • File names: Could use ., but not : or /. However, if the namespace is realized as a folder, we don't need a separator character on the file system level.
  • Package names: Not sure if we need them. But yes, dot would not work here. Note however that hyphen e.g. "seaborn08-colorblind" does not work as well (though we might be able to replace that by underscore as a workaround).
  • General identifiers: I don't see that we'd need style names as identifiers, such as styles.seaborn_xyz.

Edit: OTOH the worst that is happening if we don't use a dot now is that we have less readable style names for the old seaborn styles. We can always add dots later and even add an alias for these seaborn styles. So feel free to merge if you think you want to reserve all liberties concerning dots.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 19, 2022

I do think that having not so nice names for old (somewhat discouraged) seaborn styles isn't a big deal (as noted in your edit).

My (yet to be fully spelled out) proposal for pip-installable mplstyles is basically to map mpl.style.use("foo.bar.baz") to import foo.bar; mpl.style.use(Path(foo.bar.__file__).parent / "baz.mplstyle")) where foo.bar is a package, so there's a foo/bar/__init__.py and a foo/bar/baz.mplstyle next to it (some important details are that we don't actually need to import foo.bar but can just locate it with importlib and therefore skip any execution of Python code (*), and the implementation also needs to be adjusted for namespace packages (i.e. foo/bar/__init__.py is not actually needed).

We could say that this needs to be spelled e.g. mpl.style.use("foo.bar:baz") (somewhat similarly to pkgutil.resolve_name), but that may be more confusing for end users. Using slashes is likely worse because the front part is really a qualified package name (so that should really be dots), and "foo.bar/baz" seems a bit strange, plus you may confuse Windows users as to whether backslashes are needed or supported.

(*) As always, I don't agree that this is so important to avoid, but accept that that's probably the easiest way to get such a proposal moving forward...

@tacaswell
Copy link
Member

I lean toward taking this for now to make sure we do not paint our selves into a corner. Adding extra aliases (without removing the existing ones) is messy, but not disruptive.


Other prior art we have is that for colors we use 'tab10:' and 'xkcd:' as prefixes to conflict with the named X11 colors.

@jklymak
Copy link
Member

jklymak commented Aug 19, 2022

Yes at first I thought getting rid of the dots was strange, but agree that python library imports use the dots so if we want to import styles keeping dots out of the namespace makes sense.

@timhoffm
Copy link
Member

timhoffm commented Aug 19, 2022

Thinking again: "seaborn08" is really ugly. Let's not do this.

Instead, let's use a different name like "seaborn-legacy" or "seaborn-original". The suffix can basically be anything because we only need to make it clear it's not the same as (current) seaborn. We're not wedded to using a version number for this. If we introduce versioning of themes that will be a completely separate concept.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 19, 2022

That's fine with me too (and I don't reall care about legacy vs. original). Anyone else wants to pick the adjective?

@timhoffm
Copy link
Member

If nobody else has an opinion, I suggest to go with "legacy", but don't have a strong preference either.

@jklymak
Copy link
Member

jklymak commented Aug 20, 2022

I actually prefer versions because what happens when seaborn changes again - seaborn-more-recent-legacy is going to get confusing. I'd just spell the version seaborn_v0_8.

@timhoffm
Copy link
Member

timhoffm commented Aug 20, 2022

what happens when seaborn changes again

That does not bother us. We don't and don't intend to ship current and future seaborn styles. I think this was discussed in a dev call but can't find the reference. The motivation for renaming is clarifying that the style we are shipping is not the current seaborn style. Any name except plain "seaborn" will do.

Even if we should decide to ship seaborn or make it available as 3rd party installable package in the future that's not a problem. I very strongly advocate that if we open up the styling, we need a larger strategy for style handling, including versioning. Independent of that we'll have to support the existing names for backward compatibility.

@tacaswell
Copy link
Member

Discussed on the call, will change to seaborn-v0_8-XXXX and are on board with dropping . to preserve future flexibility as a special character.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 25, 2022

I may not be able to push a fix real quick, feel free to force push to rename that.

@tacaswell
Copy link
Member

dealing with this is on my plate for tonight. I assumed you would be ok with that @anntzer .

@anntzer
Copy link
Contributor Author

anntzer commented Aug 25, 2022

Yes :)

... as we may want to use dots later to support third-party styles, e.g.
`foo.bar` could be used to mean `foo/bar.mplstyle` where `foo` is a
regularly installed python package.
@QuLogic QuLogic merged commit 942aa77 into matplotlib:main Aug 26, 2022
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 26, 2022
@anntzer anntzer deleted the 08 branch August 26, 2022 06:21
QuLogic added a commit that referenced this pull request Aug 26, 2022
…674-on-v3.6.x

Backport PR #23674 on branch v3.6.x (Re-rename builtin seaborn styles to not include a dot.)
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. topic: styles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants