Should palettes be iterables?

[mezarque]

Currently, we use palettes just to group discrete colors together. It might be more useful to allow them to be accessed by their indices, e.g. apc.palettes.accent[0:2] might return a Palette containing the first two colors. I suppose users can do this using apc.palettes.accent.colors[0:2], but that feels a little cumbersome.

[ekiefl]

First of all, nice work you guys! The documentation is extremely helpful and thorough, and the API feels nice, and above all, the plots and color options look very nice.

Speaking as 0.5.0's likely first user here. @mezarque, I noticed this question you made based on the title "Should palettes be iterables?", because when using the API I intuitively expected Palette to have length and be iterable. After seeing the colors attribute being accessed in the quickstart guide, I realized that Palette isn't actually a color sequence, but it holds a color sequence. Easy enough, but a little confusing.

IMO, the ColorSequence abstract base class should implement length (__len__) and be iterable (__iter__ and __getitem__). This would be more in line with the class name (ColorSequence), since a sequence is a lengthy, indexable iterable.

Here are the method implementations that turn ColorSequence into a true sequence and supports your desire to have slice-based indexing:

    @overload
    def __getitem__(self, index: int) -> HexCode: ...
    
    @overload
    def __getitem__(self, index: slice) -> T: ...
    
    def __getitem__(self, index: Union[int, slice]) -> Union[HexCode, T]:
        if isinstance(index, slice):
            return self.__class__(name=f"{self.name}_slice", colors=self.colors[index])
        else:
            return self.colors[index]

    def __iter__(self):
        return iter(self.colors)

    def __len__(self):
        return len(self.colors)

The change in action:

[ins] In [2]: len(apc.palettes.primary)
Out[2]: 12

[nav] In [4]: for color in apc.palettes.primary: print(color)
#5088C5
#F28360
#3B9886
#F7B846
#7A77AB
#F898AE
#73B5E3
#FFB984
#97CD78
#C85152
#F5E4BE
#BABEE0

[ins] In [5]: apc.palettes.primary[2:5]
Out[5]:

   seaweed #3B9886
   canary  #F7B846
   aster   #7A77AB

[ins] In [12]: apc.palettes.primary.reverse().colors == apc.palettes.primary[::-1].colors
Out[12]: True

[ins] In [6]: apc.gradients.blues[:2]
Out[6]:

   lapis  #2B65A1 0.0
   aegean #5088C5 1.0

But if you look at the last example, it's not all roses. Gradient accepts a values parameter that the base __getitem__ doesn't pass, so the default fallback evenly distributes the values erroneously. The workarounds would be (1) to have Gradient overwrite the base __getitem__ with it's own, or (2) put some these dunder methods in Palette and forget about slicing a gradient.

I think both these workarounds illustrate a potential shortcoming in the inheritance relationship between ColorSequence and Gradient, namely, that it exists at all.

Operationally, the Gradient defines anchors (paired colors and values), that can create interpolated color sequences. To me this is not itself a color sequence--it's a color sequence factory. Sure, it's anchors are lengthy, indexable, and iterable, but imo the gradient should have none of those characteristics.

I'm really pulling on the thread here, so I would like to preface this by saying I think the code is perfectly fine and we probably shouldn't touch it, but I would prefer composition over inheritance here, as it solves all the issues that led me to unravel this sweater. Something like this:

@dataclass
class Anchor:
    color: HexCode
    value: float


class Gradient:
    def __init__(self, name: str, anchor_colors: List[HexCode], anchor_values: Union[List[float], None] = None):
        """
        A Gradient is a sequence of pairs of HexCode objects and numeric values
        from 0 to 1 that represent the position of each color in the gradient.

        Args:
            name (str): the name of the gradient
            colors (list): a list of HexCode objects
            values (list): a list of float values corresponding
                to the position of colors on a 0 to 1 scale.
        """
        self.name = name

        if anchor_values is not None:
            (...)

        self.anchors = [Anchor(color, value) for color, value in zip(anchor_colors, anchor_values)]

    @property
    def num_anchors(self) -> int:
        return len(self.anchors)

Not inheriting from a sequence, Gradient has no length and can't be iterated, or indexed (which I think makes sense), but it does have the property num_anchors and its anchors can be iterated and index. And of course any palettes it generates are sequences. As it happens, this anchors attribute now closely resembles the docstring description. That's probably a good sign that we're converging onto something good.

After massaging Gradient's methods to use anchors, the Gradient class works even though it no longer subclasses ColorSequence. That's because the ColorSequence had no method implementations (besides the _get_longest_name_length one-liner), and served only to constrain Gradient with abstract methods.

With Gradient no longer constrained to act like a color sequence, we can just get rid of the ColorSequence class altogether and dump whatever we need (__iter__, __len__, __getitem__) into the true color sequence, Palette. In general, abstract base classes are more warranted when there exists functionality that can process a variety of object types agnostically, so long as they satisfy the abstract class, but I don't see that in this codebase.

Again, I think it's great as is, and I'm really enjoying using it. But I wanted to sketch out a path for how one would cleanly enable the feature described in this issue. Alongside that, I also sketched out some hacky alternatives that could be implemented in the near term if we really want Palettes to be sequences.

[mezarque]

Thanks so much for taking the time to try out the package and think about this issue! Overall, I think your suggestions are really helpful. I agree that Gradient needn't inherit from ColorSequence, and that disentangling them would allow us to implement iterable methods for Palette. I'm not sure that we need Anchor to replace the underlying structure of the Gradient class - is the desire mostly to have the attributes of Gradient be sufficiently different from Palette so as to avoid confusion?

That detail aside, I think there's a handful of places that things would need to be rearranged in the package if Gradient doesn't inherit from ColorSequence, particularly for some type checks that we do when registering things to matplotlib, but otherwise this should work. Feel free to open a PR for these changes if you'd like, or we can explore implementing this for the next minor version.

[ekiefl]

Nice. I'll open a PR the next time I have some down time.

I'm not sure that we need Anchor to replace the underlying structure of the Gradient class - is the desire mostly to have the attributes of Gradient be sufficiently different from Palette so as to avoid confusion?

I was more just thinking of how I would like to structure the class in a world where it doesn't inherit from ColorSequence, and thought of a data structure that reflected the current docstring description. But upon further thought, I agree we don't need to replace the underlying structure... But it could be useful to instead have a property that zips colors and values together in a list of color-value two-ples, since that is an accepted format for passing custom color gradient definitions to plotly, and also the way we create matplotlib LinearSegmentedColormap objects, as illustrated in to_mpl_cmap:

    def to_mpl_cmap(self):
        colors = [(value, color.hex_code) for value, color in zip(self.values, self.anchor_colors)]
        return mcolors.LinearSegmentedColormap.from_list(
            self.name,
            colors=colors,
        )

[mezarque]

That sounds good to me! In a previous version we had implemented something like this (particularly to accommodate usage with plotly), but decided to take it out to keep things a bit tidier. Happy to review a PR whenever you have time - no rush!