I would not overload the signature. A typical strategy would be to have the canoncial signature and create a factory classmethod for the other signature:

def __init__(colorizer):
    self.colorizer = colorizer

@classmethod
def from_norm_and_cmap(cls, norm, cmap):
    return cls(Colorizer(norm, cmap)

AFAICT, the situation here is a bit different, because users do not instantiate ColorizingArtist directly, and hence, they won't use the factory method. We could do one of the two options:

• only implement def __init__(colorizer). If subclasses need to initialize from norm + cmap, they'd simply do

  ColorizingArtist.__init__(Colorizer(norm, cmap))
  

• create an alternative initializer

  def _init_from_norm_and_cmap(norm, cmap):
      self.colorizer = Colorizer(norm, cmap)
  

Given that we'll only have a handfull of subclasses and the solution is quite simple, I'm leaning towards the first option.

The first alternative makes sense to me.

However, we still need the logic where the user can pass a Colorizer object as part of the input to the plotting function.

I implemented this with the following function:

def _get_colorizer(cmap, norm):
    """
    Passes or creates a Colorizer object.

    Allows users to pass a Colorizer as the norm keyword
    where a artist.ColorizingArtist is used as the artist.
    If a Colorizer object is not passed, a Colorizer is created.
    """
    if isinstance(norm, Colorizer):
        if cmap:
            raise ValueError("Providing a `cm.Colorizer` as the norm while "
                             "at the same time providing a `cmap` is not supported.")
        return norm
    return Colorizer(cmap, norm)

Such that the initiation of a ColorizingArtist in e.g. Collection is:

        artist.ColorizingArtist.__init__(self, colorizer._get_colorizer(cmap, norm))

Do we really want to pass a Colorizer via the norm parameter in public API? This feels very hacky. I'm inclined to add a colorizer parameter instead, even if that means we have mutually exclusive parameters. (Note that even with the hack, we'd have to live with partially mutually exclusive parameters, because you cannot pass norm=mycolorizer, cmap='jet').

I have no strong opinions, and am inclined to agree with you @timhoffm.
Since this is a change to the call signature of the plotting functions, we should bring it up at a weekly meeting. @story645 could you add it on the agenda for the next meeting?

(My aversion to new keyword arguments is a result of a discussion at a weekly meeting a long time ago about multivariate color mapping, and how we do not want that to increase the number of keyword arguments, but this is a different case entirely.)

Sorry I didn't see this til now 😦 but also anyone can add stuff to the agenda (sorry me and Kyle didn't make that clear)! And also this got resolved as yes add new kwarg, I think.

As the colorizing artist gets built out/in later PRs, is the plan that the inherit from Shim goes away?

No. The shim provides the backward-compatible interface for the ScalarMappaple API. It‘s needed for the foreseeable future, but it’s reasonable to keep it separated from the ColorizingArtist API.

So where I'm confused is that I understand the compatibility layer as public API for downstream libraries, I'm confused about compatibility layer for private API where we can use the thing directly.

I mean down the line, I understand for this and some foreseeable PRs the shim being here to show that the functionality is being maintained w/o having to reimplement everything.

There's a utility function for this, probably https://matplotlib.org/devdocs/api/_api_api.html#matplotlib._api.check_in_list

Rather https://matplotlib.org/devdocs/api/_api_api.html#matplotlib._api.check_getitem

This code mirrors the existing code for ScalarMappable (cm.py lines 545→552), but I'll see if I can shift it to use the _api or if there is some reason why that has not been implemented for ScalarMappable.

Two points:

1. This will change the error message from:

" Invalid norm str name; the following values are supported: 'linear', 'log', 'symlog', 'asinh', 'logit', 'function', 'functionlog'

to

' ' is not a valid value for norm; supported values are 'linear', 'log', 'symlog', 'asinh', 'logit', 'function', 'functionlog'

Which is slightly misleading, as norm supports other inputs than strings. We can mitigate this by intercepting the error message and replacing value with string, but this requires an additional try except block.

2. I think this check naturally belongs in _auto_norm_from_scale, a function which is currently called by:

        elif isinstance(norm, str):
            try:
                scale_cls = scale._scale_mapping[norm]
            except KeyError:
                raise ValueError(
                    "Invalid norm str name; the following values are "
                    f"supported: {', '.join(scale._scale_mapping)}"
                ) from None
            norm = _auto_norm_from_scale(scale_cls)()

which I think should be replaced by:

        elif isinstance(norm, str):
            norm = _auto_norm_from_str(norm)()

(note the change in function name from _scale to _str)
This function then incorporates the check.

def _auto_norm_from_str(norm_str):
    scale_cls = _api.check_getitem(scale._scale_mapping, norm=norm_str)
    ...

@timhoffm @story645 Let me know your opinions on point 1 and 2 above, and I will implement it in this PR.
Point 2. would be a useful change with regards to making a 'MultiNorm' in a later PR.

I agree the message would be slightly misleading. OTOH pushing this down does not make much better for the user. Thy still get the same exception message. Only the call stack is different. While the message in the deeper context is correct, it's further away from the public API and thus more difficult to trace back.

An alternative would be expanding _check_get_item so that we can do _api.check_getitem(scale._scale_mapping, norm=norm_str, _type="str") and get "' ' is not a valid str value ...", which would imply there may be other valid types.

I believe in practice, it does not matter. If the user has passed an invalid str, they most likely have a typo and still wanted to pass a str name. It's unlikely that the information being able to pass Norms directly would be helpful. To keep things simple, I slighly favor just using _check_get_item as is in original context (this does not factor in any possible future needs). But overall no strong opinion.

Point 2. would be a useful change with regards to making a 'MultiNorm' in a later PR.

Would it be easy to make this change inside the MultiNorm PR or better to have it here?

I think it's better to have it here, with the changes in #28690 that means we only have to move this function once.

Would changing the get item check method in the way Tim described achieve the same ends? If so do that.

If not, unfortunately I think moving it twice but making the change when folks can see why it helps is gonna be an easier sell.

are we sure that this will always have a .norm?

I'm thinking it will always have a norm, as I think that simplifies the implementation.
However, that norm might be NoNorm.

The implementation here mirrors that of ScalarMappable, which always has a Normalize and Colormap object, and I think this is sensible. For images that are already RGB(A) a norm (and cmap) is technically unnecessary, but I believe the overhead of creating them is negligible.

This has to go after the * or we break API (even though passing 10 positional arguments is a Bad Idea ™️ if someone was doing that we would now get the value of alpha as colorizer).

Side-remark: We should consider tightening on the kw-only settings. It helps the understanding to collect related parameters next to each other. I'd be willing to force people with Bad Ideas™️ to adjust in this case.

But this doesn't help for now because we'd need to go through the deprecation cycle. So @tacaswell's comment is valid anyway.

Thank you for the feedback, I was unsure if it was OK to break the list of args for people with Bad Ideas™️, but I guess we should cater to everyone :)