Skip to content
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

[Typing] px.timeline.color_continuous_scale accepts also list[tuple[float,str]], not only list[str] #4069

Open
stdedos opened this issue Feb 13, 2023 · 6 comments
Assignees
Labels
bug something broken P3 not needed for current cycle

Comments

@stdedos
Copy link

stdedos commented Feb 13, 2023

https://plotly.com/python-api-reference/generated/plotly.express.timeline.html

color_continuous_scale (list of str) – Strings should define valid CSS-colors This list is used to build a continuous color scale when the column denoted by color contains numeric data. Various useful color scales are available in the plotly.express.colors submodules, specifically plotly.express.colors.sequential, plotly.express.colors.diverging and plotly.express.colors.cyclical.

However, here the example shows:

https://github.com/plotly/plotly.py/blob/182fff29bce98c30ab516242d4e01d2d2d0bee0f/doc/python/colorscales.md#explicitly-constructing-a-color-scale

@itsmegarvi
Copy link

So, we need to change color_continuous_scale (list of str) to color_continuous_scale (list of str and list of a tuple of float and string). If I have missed something please guide me.

@stdedos
Copy link
Author

stdedos commented Feb 17, 2023

Here's the syntactic type I've defined:

PlotlyColorScale = Union[list[str], list[tuple[Union[float, int], str]]]
# or, for py3.10/3.11
PlotlyColorScale = list[str] | list[tuple[float, str]]]
# ... but for mypy, it should be `float == float | int` (https://github.com/python/mypy/issues/3186)

@nicolaskruchten
Copy link
Contributor

Yes, the docstring here is a bit behind the API expansion, but the error message you get if you give it something wrong is complete and correct, as below. Note that a simple string is accepted as well (it is looked up case-insensitively in a list of constants shown below), with or without a trailing "_r" to reverse it.

The 'colorscale' property is a colorscale and may be
    specified as:
      - A list of colors that will be spaced evenly to create the colorscale.
        Many predefined colorscale lists are included in the sequential, diverging,
        and cyclical modules in the plotly.colors package.
      - A list of 2-element lists where the first element is the
        normalized color level value (starting at 0 and ending at 1),
        and the second item is a valid color string.
        (e.g. [[0, 'green'], [0.5, 'red'], [1.0, 'rgb(0, 0, 255)']])
      - One of the following named colorscales:
            ['aggrnyl', 'agsunset', 'algae', 'amp', 'armyrose', 'balance',
             'blackbody', 'bluered', 'blues', 'blugrn', 'bluyl', 'brbg',
             'brwnyl', 'bugn', 'bupu', 'burg', 'burgyl', 'cividis', 'curl',
             'darkmint', 'deep', 'delta', 'dense', 'earth', 'edge', 'electric',
             'emrld', 'fall', 'geyser', 'gnbu', 'gray', 'greens', 'greys',
             'haline', 'hot', 'hsv', 'ice', 'icefire', 'inferno', 'jet',
             'magenta', 'magma', 'matter', 'mint', 'mrybm', 'mygbm', 'oranges',
             'orrd', 'oryel', 'oxy', 'peach', 'phase', 'picnic', 'pinkyl',
             'piyg', 'plasma', 'plotly3', 'portland', 'prgn', 'pubu', 'pubugn',
             'puor', 'purd', 'purp', 'purples', 'purpor', 'rainbow', 'rdbu',
             'rdgy', 'rdpu', 'rdylbu', 'rdylgn', 'redor', 'reds', 'solar',
             'spectral', 'speed', 'sunset', 'sunsetdark', 'teal', 'tealgrn',
             'tealrose', 'tempo', 'temps', 'thermal', 'tropic', 'turbid',
             'turbo', 'twilight', 'viridis', 'ylgn', 'ylgnbu', 'ylorbr',
             'ylorrd'].
        Appending '_r' to a named colorscale reverses it.

@stdedos
Copy link
Author

stdedos commented Feb 17, 2023

Yes, the docstring here is a bit behind the API expansion, but the error message you get if you give it something wrong is complete and correct,

Well ... REPL is not the nicest way to develop with unknown libraries 😅

  1. typing/stubbing would've been amazing (Add type annotation stubs #1103, Add type information for Pylance #3401, (Include type hints in function parameters dash#2233), ...)
  2. then it's documentation
  3. ... then it's pulling your hair "why does documentation say XYZ, but my developer here did ABC ⁉️ ⁉️ ⁉️ ... and it's working???"
  4. Then ... it's giving wrong values as input so that you can get an error message with the accepted types?
    While I commend your effort, on code that's working, that does not provide the same value (as when developing)

@nicolaskruchten
Copy link
Contributor

Yep, there is definitely room for improvement here. My feeling is that maintaining type-hint information is much harder than keeping the docstrings up to date, and we're already struggling to keep the docstrings up to date, so I can't commit to adding and maintaining type hints at the moment. We would gladly accept a PR that makes the docstrings from Plotly Express more complete. The relevant file is here: https://github.com/plotly/plotly.py/blob/master/packages/python/plotly/plotly/express/_doc.py#L316

@stdedos
Copy link
Author

stdedos commented Feb 17, 2023

My feeling is that maintaining type-hint information is much harder than keeping the docstrings up to date, ...

We certainly disagree on that 😅

pre-commit + mypy (especially now that it moved to v1.0.0) can go a long way of keeping the code up-to-date. Documentation is text; there's no validating it, certifying it, "tying it down".

What led me here is "flexible usage" of the plotly API, lack of documentation (and also flexible usage of my code base before calling the said API).

mypy is certainly a difficult demon to master, especially when supporting all "smart" cases - but it may be so just because it is actually solving the complex problem of "over-flexibility allowed" 🥵

@gvwilson gvwilson added P3 not needed for current cycle bug something broken and removed documentation labels Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken P3 not needed for current cycle
Projects
None yet
Development

No branches or pull requests

6 participants