silx.gui.plot.PlotWidget.addCurve: Fixed progression in color, linestyle #4138

EdgarGF93 · 2024-06-14T15:10:53Z

Checklist:

The PR title is formatted as: <Module or Topic>: <Action> <Summary> (see contributing guidelines)

From #4133

This PR fixed the way the addCurve requests color and style (examples)
Test code:

from silx.gui.plot import Plot2D
import numpy as np
from PyQt5.QtWidgets import QApplication, QMainWindow

if __name__ == "__main__":
    app = QApplication([])
    mw = QMainWindow()
    plot = Plot2D()

    x = np.linspace(0,10,10)

    y = [x*ii for ii in range(20)]

    for _ in range(20):
        color, style = plot._getColorAndStyle()
        print(f"color index is {plot._colorIndex}, style index is {plot._styleIndex}")
        plot.addCurve(x,y[_], legend=_ , 
                      #color='black',
                      color=color,
                      linestyle=style,
                     # linestyle='-',
                      )

    mw.setCentralWidget(plot)
    mw.show()
    app.exec_()

If a color is requested before to use in the plot (and maybe other plots or markers), it completes the 10 colors of the colormap before changing the linestyle

color index is 1, style index is 0
color index is 2, style index is 0
color index is 3, style index is 0
color index is 4, style index is 0
color index is 5, style index is 0
color index is 6, style index is 0
color index is 7, style index is 0
color index is 8, style index is 0
color index is 9, style index is 0
color index is 0, style index is 1
color index is 1, style index is 1
color index is 2, style index is 1
color index is 3, style index is 1
color index is 4, style index is 1
color index is 5, style index is 1
color index is 6, style index is 1
color index is 7, style index is 1
color index is 8, style index is 1
color index is 9, style index is 1
color index is 0, style index is 2

Before the PR, addCurve request the color again (not to be used) but advances +1, so the colormap finish after 5 iterations:

color index is 1, style index is 0
color index is 3, style index is 0
color index is 5, style index is 0
color index is 7, style index is 0
color index is 9, style index is 0
color index is 1, style index is 1
color index is 3, style index is 1
color index is 5, style index is 1
color index is 7, style index is 1
color index is 9, style index is 1
color index is 1, style index is 2
color index is 3, style index is 2
color index is 5, style index is 2
color index is 7, style index is 2
color index is 9, style index is 2
color index is 1, style index is 3
color index is 3, style index is 3
color index is 5, style index is 3
color index is 7, style index is 3
color index is 9, style index is 3

Now, since there is a method -_getStyle, decoupled from color, if a color ('black') is passed to addCurve, it will add +1 to the linestyle index.

color index is 0, style index is 0
color index is 0, style index is 1
color index is 0, style index is 2
color index is 0, style index is 3
color index is 0, style index is 0
color index is 0, style index is 1
color index is 0, style index is 2
color index is 0, style index is 3
color index is 0, style index is 0
color index is 0, style index is 1
color index is 0, style index is 2
color index is 0, style index is 3
color index is 0, style index is 0
color index is 0, style index is 1
color index is 0, style index is 2
color index is 0, style index is 3
color index is 0, style index is 0
color index is 0, style index is 1
color index is 0, style index is 2
color index is 0, style index is 3

Before the PR, it waits 10 color indexes to change the style:

color index is 0, style index is 0
color index is 1, style index is 0
color index is 2, style index is 0
color index is 3, style index is 0
color index is 4, style index is 0
color index is 5, style index is 0
color index is 6, style index is 0
color index is 7, style index is 0
color index is 8, style index is 0
color index is 9, style index is 0
color index is 0, style index is 1
color index is 1, style index is 1
color index is 2, style index is 1
color index is 3, style index is 1
color index is 4, style index is 1
color index is 5, style index is 1
color index is 6, style index is 1
color index is 7, style index is 1
color index is 8, style index is 1
color index is 9, style index is 1

closes #4133

t20100 · 2024-06-18T15:41:44Z

Thanks for the PR!

I think the rotation of the color and style should be handled differently between the 3 cases:

no color nor linestyle
only color
only linestyle

This code

from silx import sx
w = sx.plot()

w.addCurve([1, 2], [0, 1], legend="1")
w.addCurve([1, 2], [0, 2], legend="2")
w.addCurve([1, 2], [0, 3], legend="3", color="black")
w.addCurve([1, 2], [0, 4], legend="4")

displays:

So passing only the color rotate the linestyle.

I would propose the following behavior:

no color nor linestyle: rotate color and style as it is currently done
only color: use - linestyle all the time or eventually rotate linestyle but independently from the other cases
only linestyle: rotate color. To me it is OK to rotate with the "no color nor linestyle" case. A smarter implementation could check what (color, linestyle) is already used an avoid those... but it sounds to complicated to me. An other option could be to always use the same color in this case (e.g., black) to have a similar behavior as the "only color" case with always the same linestyle, but as a user I would expect the colors to rotate...

What do you think?

t20100 · 2024-06-19T08:13:37Z

Also, we need to keep the behavior as compatible as possible in order to limit unexpected changes for application using it.

EdgarGF93 · 2024-06-19T13:16:52Z

The way I did it:

If no color and style is provided, rotate both with the same method (_getColorAndStyle) as before
If color provided, it rotates the style (only 4 styles available) After four curves, it uses the same color-style
If style is provided, it rotates the color. After 10 curves, it takes the same color-style
If both are provided, it uses it but it does not request them again (that was the reason I opened the issue).

I think this is the logical way, if the user provides a constant color and style, (s)he is aware that eventually the color-style is going to be duplicated. Is this behavior something to avoid at all? Because in that case we can easily use only one method (_getColorAndStyle), both indices (color and style) are always connected, and then repetition of curves is always avoided.
I'm voting for keeping the frozen color-style (there has be to a situation in which the user would want to freeze the style)

t20100 · 2024-06-19T14:40:51Z

if the user provides a constant color and style, (s)he is aware that eventually the color-style is going to be duplicated.

Agreed.

To me the key issue is that if both color and style are provided, it should not rotate the default.
Now when changing this, we have to keep an eye on providing a similar behavior for backward compatibility at the same time as improving this behavior.

The fact that adding a curve with a color rotates the style for the next curve without color (see code in #4138 (comment)) sounds hard to understand without knowing the internals, thus my previous propositions reorganised here:

If no color and style is provided, rotate both with the same method (_getColorAndStyle) as before

If both are provided, it uses it but it does not request them again (that was the reason I opened the issue).

Agreed.
Regarding the remaining cases:

If color provided:
If the styles rotates there, it should not interfere with the " no color and style is provided".
I'm in favor of always using a solid line style here, because rotating the style would mean keeping a record of which style was last used for which color (not worth it IMO).
If style is provided, it rotates the color. After 10 curves, it takes the same color-style
OK for me, and I would use _getColorAndStyle
@axelboc suggested using the previously used default color (i.e. take the color but do not rotate) unless the current default style is the same, and in this case do a rotation.

Anyone has comments on this discussion?

EdgarGF93 · 2024-06-19T14:58:48Z

Ok, yes, I fully agree with not-rotating the style if color is provided, especially because is keeping the same behavior as before the PR

EdgarGF93 · 2024-06-19T16:04:52Z

I think this way it stays as similar as before but solving the index bug and the rotating style discussion

If color is provided, use that color and the current linestyle, without requesting color-style
Any other situation proceeds the same way as before the PR, rotating both color and style

t20100

LGTM, thanks!

I made a PR on your branch to include in this PR if it's OK for you to also fix the issue for the addHistogram method

silx.gui.plot.PlotWidget.addHistogram: Fixed progression in color, linestyle

EdgarGF93 · 2024-06-20T08:26:04Z

perfect, thanks

edgar added 2 commits June 14, 2024 16:39

fixed progression color, linestyle

7f0b360

fix case

2f89808

EdgarGF93 changed the title ~~fixed progression color, linestyle~~ silx.gui.plot.PlotWidget: fixed progression color, linestyle Jun 14, 2024

EdgarGF93 changed the title ~~silx.gui.plot.PlotWidget: fixed progression color, linestyle~~ silx.gui.plot.PlotWidget.addCurve: Fixed progression in color, linestyle Jun 14, 2024

EdgarGF93 mentioned this pull request Jun 14, 2024

addCurve advances color index even if color is provided #4133

Closed

more readable

133850b

four cases

72f4de8

edgar added 3 commits June 19, 2024 17:51

keep linestyle

ffb6fc0

typo

a8016b9

keep it simple

3a1cbc2

Do not rotate colors and style for addHistogram if color is provided

73c5c8e

t20100 approved these changes Jun 20, 2024

View reviewed changes

edgar and others added 3 commits June 20, 2024 10:16

fixed index bug on addHistogram

3a57d9a

Merge branch '4133_advance_color_linestyle' into pr4138

071c7c6

Merge pull request #1 from t20100/pr4138

2d9b9a1

silx.gui.plot.PlotWidget.addHistogram: Fixed progression in color, linestyle

t20100 merged commit 01827c8 into silx-kit:main Jun 20, 2024
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

silx.gui.plot.PlotWidget.addCurve: Fixed progression in color, linestyle #4138

silx.gui.plot.PlotWidget.addCurve: Fixed progression in color, linestyle #4138

EdgarGF93 commented Jun 14, 2024 •

edited by t20100

Loading

t20100 commented Jun 18, 2024

t20100 commented Jun 19, 2024

EdgarGF93 commented Jun 19, 2024 •

edited

Loading

t20100 commented Jun 19, 2024

EdgarGF93 commented Jun 19, 2024 •

edited

Loading

EdgarGF93 commented Jun 19, 2024 •

edited

Loading

t20100 left a comment

EdgarGF93 commented Jun 20, 2024

silx.gui.plot.PlotWidget.addCurve: Fixed progression in color, linestyle #4138

silx.gui.plot.PlotWidget.addCurve: Fixed progression in color, linestyle #4138

Conversation

EdgarGF93 commented Jun 14, 2024 • edited by t20100 Loading

t20100 commented Jun 18, 2024

t20100 commented Jun 19, 2024

EdgarGF93 commented Jun 19, 2024 • edited Loading

t20100 commented Jun 19, 2024

EdgarGF93 commented Jun 19, 2024 • edited Loading

EdgarGF93 commented Jun 19, 2024 • edited Loading

t20100 left a comment

Choose a reason for hiding this comment

EdgarGF93 commented Jun 20, 2024

EdgarGF93 commented Jun 14, 2024 •

edited by t20100

Loading

EdgarGF93 commented Jun 19, 2024 •

edited

Loading

EdgarGF93 commented Jun 19, 2024 •

edited

Loading

EdgarGF93 commented Jun 19, 2024 •

edited

Loading