-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
[MNT]: Move norms to from colors.py to separate file norms.py #28690
Comments
At the weekly meeting Aug. 8th 2024, this issue was disussed, and there was a general consensus that this is a good idea from a maintenance/development point of view, but there are potential drawbacks with regards to backwards compatibility. Three options were discussed:
At the meeting there was a general agreement that the current structure of see the meeting notes https://hackmd.io/@matplotlib/HkY6emxs2 |
I fully agree that the current structure and naming is confusing. I never remember where the different parts are. I think the All colormapping-related stuff should be separate. Disregarding for a moment where we are now, there are fundamentally three approaches one could take:
From a user and API perspective, I find one large How to get there?
|
I fully agree that the current structure is confusing. I can never remember where I find the individual parts. I suggest the following refactoring:
How to get there:
The backward-compatibility will be indefinite for now. |
I would just like to confirm that I understand you correctly, does this agree with what you have in mind?
|
I'm inclined to keep content out of Edit: Should we use singular module names |
I see, then this should probably all wrap into #28658. I would personally prefer plural names for the files that should include multiple classes in the same family:
vs:
But
If you think this is too granular, then we do everything singular. |
Love the broad strokes, am worried that since we already associate What about keeping colors as the broad grouping and then move out all those utils functions into
ETA: what I mean is a structure like:
|
I see your point @story645 but I find I'm leaning towards from matplotlib import colormapping as mcm and while |
By convention, cm is imported as mcm 😓 A shorter alternative is coloring but my guess is that coloring and colors will confuse everyone. And yeah, I'm somewhat neutral/am not gonna put up a fight on this b/c I get that you're trying to isolate this one very specific pipeline. |
I see the point in suggesting to collect everything under colors. It would also have the advantage that norms and colormap classes would not change their location. These are strong advantages. However, I‘m feeling uneasy with the idea. It feels imbalanced: First, the non-mapping code is small compared to the mapping architecture. It‘s a bit lost. Second, I‘d like the public namespace be purely re-imports (also thinking in docs organization: These are the different parts documented in detail on the sub-pages). Adding documentation for the few non-mapping functions is imbalanced). OTOH it also does not make sense to move that documentation to a public subnamespace (matplotlib.colors.colors - or slightly better matplotlib.colors.utils). Second thoughts: It might be possible to get the structure resolved by de-emphasizing the sub-namespaces and making the colors namespace docs an index similar to https://matplotlib.org/devdocs/api/axes_api.html#module-matplotlib.axes, where the non-mapping functions are one section and link to individual doc pages per function. But I‘d have to have a closer look at that. |
To keep things simple and better separated, I suggest to start with the separate |
So I made an attempt at implementing this.
where I found some drawbacks to this structure.
At the core of this is the fact that different aspects of colors.py and cm.py are heavily interconnected. While these are not insurmountable issues, I don't see a solution that does not rely on workarounds (i.e. calling imports in file A from file B to avoid circular imports), or an even more brutal refactoring/renaming operation. I raised this issue because I propose the following:
from ._norms import \
(Normalize, NoNorm, TwoSlopeNorm, CenteredNorm, # noqa: F401
FuncNorm, LogNorm, SymLogNorm, AsinhNorm,
make_norm_from_scale, PowerNorm, BoundaryNorm)
from ._colormap_classes import \
(Colormap, LinearSegmentedColormap, ListedColormap) # noqa: F401 All access to the norms and colormaps still goes through Additionally this brings us closer to the structure @story645 suggested, with |
I'm only half following this, but I constantly need to remind myself where the norms are stashed. I wouldn't look for them under "color". At least now they are under "cm" which has "mapping" in the name. I don't see what the problem with having a top-level norms.py is. While I agree that 99.99% of the time they are used for colormapping, they are just mathematical transforms. |
I agree here - I proposed colors as top level as an alternative to The rcParam issue can be solved via delayed evaluation til run time kinda like how we manage backends, but I seriously don't think it's worth it. matplotlib/lib/matplotlib/__init__.py Line 760 in 054fa9c
|
I completely agree, both on norms being hard to find and that norms are their own thing. |
The problem I see is that we currently have 60 subnamespaces. If we through the ~6 colormapping related namespaces flat in, it's harder to see that they play together. That's like throwing a jigsaw puzzle consisting of 6 pieces into a large bag with other pieces - you'll not find the pieces belonging together again. In my eyes, the "just mathematical transform" is not an argument. While technical true, we clearly associate it with the colormapping. IMHO that's helps users build a mental model of the architecture. Also, I would not exclude that we might want to add aspects to norms in the future that are colormapping specific and break the "just mathematical transform" assumption. I also would strongly advise users not to use norms for other purposes than colormapping. It's an architecture shortcoming if we cannot organize the module structure as we would like to due to technical reasons (dependencies). I still have to determine whether this indicates that our planned structure was not good, or whether we've issues in the current organization of code. |
As an end-user, I only use a very few of those 60 though - However, occasionally I will use a Ideally I'd just make norms available via pyplot or top-level matplotlib. However, persisting in keeping them in |
Totally agree, it’s good you usually only need one or very few imports. But if you want to know where to find something or how the library is structured, https://matplotlib.org/devdocs/api/index.html#modules is a canonical entry point.
That’s not the topic. What the current proposal does, is enabling @trygvrad to continue with the Colorizer changes without having to decide right now where to put norms eventually. There will be final module structure considerations before the official release. |
Summary
Currently the data→norm→color pipeline involves two files:
colors.py
:Colormap
and subclasses ofColormap
Normalize
and subclasses ofNormalize
cm.py
:ScalarMappable
(holds data, norm and colormap)In this structure, each file contains multiple parts of the pipeline, and it is not intuitive which part of the pipeline is located where.
With PR #28658 we are creating
colorizer.py
and the newColorizer
object to improve the data→norm→color pipeline.This has the added benefit that it greatly simplifies
cm.py
, which now only contains the colormap registry (and ScalarMappable for compatibility reasons).Proposed fix
This issue suggests moving the norms to a separate file in order to clean up
colors.py
. This, together with #28658 would lead to the following structure:colors.py
:Colormap
and subclasses ofColormap
norms.py
Normalize
and subclasses ofNormalize
cm.py
:colorizer.py
Colorizer
(handles the data→norm→color pipeline)With this structure, each file only contains a single part of the pipeline.
This issue came up because I was looking to implement multivariate color mapping (see issue #14168, and PRs #28454, #28658, [#28428]) , and for this we will need at least one more norm (i.e.
MultiNorm
).The text was updated successfully, but these errors were encountered: