silx-kit / silx

silx toolkit
http://www.silx.org/doc/silx/latest/
MIT License
119 stars 70 forks source link

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

Closed EdgarGF93 closed 2 weeks ago

EdgarGF93 commented 3 weeks ago

Checklist:


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_()
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: image

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

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

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 commented 2 weeks ago

Thanks for the PR!

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

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:

image

So passing only the color rotate the linestyle.

I would propose the following behavior:

What do you think?

t20100 commented 2 weeks ago

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

EdgarGF93 commented 2 weeks ago

The way I did it:

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 commented 2 weeks ago

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 https://github.com/silx-kit/silx/pull/4138#issuecomment-2176415182) 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:

Anyone has comments on this discussion?

EdgarGF93 commented 2 weeks ago

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 commented 2 weeks ago

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

EdgarGF93 commented 2 weeks ago

perfect, thanks