Add "left" and "right" step Modes

[cpascual]

stepMode is currently either True or False. If it is True,
it requires the user to make len(x) = len(y)+1. This is
inconvenient because it makes it difficult to change the
stepMode on a given curve (just as one would change, e.g.,
its color).

This commit extends the current situation by introducing
two more step modes: "lstep" and "rstep", which do not require
passing an extra x value. In turn, this modes associate each
y value to either the left or the right boundary of the step.

For example, the "rstep" mode is handy when plotting "live"
digital signals in which x,y data pairs are appended as they
are read.

This commit does not modify the behavior in case of stepMode=True

[cpascual]

Here an example:

import pyqtgraph as pg
from pyqtgraph.Qt import QtGui
import numpy as np

app = QtGui.QApplication([])
p = pg.PlotWidget()
p.addLegend()
x = y = np.arange(5)

p.plot(x=x, y=y, symbol="t", stepMode=False, name="False", pen="r")
p.plot(x=x, y=2+y[:-1], symbol="t", stepMode=True, name="True", pen="g")
p.plot(x=x, y=4+y, symbol="t", stepMode="rstep", name="rstep", pen="c")
p.plot(x=x, y=6+y, symbol="t", stepMode="lstep", name="lstep", pen="m")

p.show()
app.exec_()

... and its result:

[cpascual]

Hi, sorry for this shameless "bumping", but... can someone (@campagnola ?) comment on whether this contribution may be considered for inclusion or if there is something I should do to improve it?

(I just want to know because if it is going to be rejected I should find another way to implement this in my own project)

[ixjlyons]

This seems like a nice feature to me, and the API seems good in particular keeping backward compatibility. I'm inclined to accept but thoughts from others would be great.

A couple notes:

Using "step" in the possible values seems redundant. Maybe stepMode="left" and stepMode="right"? Just a thought, I don't feel strongly about it.

I would actually emphasize documentation of stepMode in the PlotDataItem class, and you could either copy the text to PlotCurveItem or refer to stepMode in PlotDataItem. The reason is the scatter plot item may be impacted by the step mode as well as the curve item. Also, people are likely (I think) to land on the PlotDataItem reference page from plot -> PlotItem.plot -> PlotDataItem.__init__ in the docs.

When stepMode == True, it's the x value that's taken as the midpoint between provided x values, not y (y values are used as given).

And a last nitpick, I think it'd be prudent to add a comment above the line in PlotCurveItem.updateData where it says if self.opts['stepMode'] is True. Someone in the future might be inclined to remove is True, but that's actually a useful distinction now that stepMode can be a string where that condition doesn't apply

[cpascual]

Thanks a lot @ixjlyons for the review. I just pushed 4 commits which, I think, address all your comments.

Please note that I included a "added in version 0.12.0" for the left and right step modes in the PlotDataItem docs, but of course I am not sure if the specific version number is right. I did it in a separate commit so that it can easily be reverted.

[j9ac9k]

Should this start with (bool or str or None) ?

[j9ac9k]

I don't have anything to add other than a thank you for submitting this feature @cpascual, I appreciate you ensuring the addition to this feature does not impact the current implementation. I must admit the API feels a bit weird that it is an Optional[Union[bool, str]] but I'm not sure of a better way of doing it (unless we introduced a stepType (or something analogous) kwarg, that defaults to standard which the existing behavior, and has lstep and rstep as potential options.

I'm just throwing this idea out there to see what folks think, I'm not requesting any changes be made.

[ixjlyons]

Regarding the weird mix of types accepted (particularly evident with is True being meaningful in the code), I was thinking there could be a "mid" option so True could be more of a legacy option (with the possibility of being deprecated and removed over time).

[cpascual]

I can do the "mid" option implementation.
I didn't do it originally because I was after the minimum possible changes in order to be super-safe in respecting backwards-compat. But I think it is quite safe to do that.

[cpascual]

d8b71e5 adds the "mid" option and marks usage of booleans as a legacy API

[cpascual]

I also added 3997532 to prevent that any other string (e.g. a misspell like "letf") ends up treated the same as "mid" or True.

[dgoeries]

I very much like this feature. I only have a nitpick and would call it center instead of mid.

[cpascual]

@dgoeries :

would call it center instead of mid.

17cab0f does it.

I originally called it "mid" as per @ixjlyons suggestion. Since it is a matter of taste, I leave it up to the integrator to revert this last commit if he/she prefers "mid" over "center".

[dgoeries]

thanks for the updates!

[j9ac9k]

I would go ahead and remove the modes added in version 0.12.0; we'll likely have this in 0.11.1....

[cpascual]

73542aa does it

[j9ac9k]

I would also consider adding a DeprecationWarning if stepMode is True

[cpascual]

2335e29 does it.

Note 1: I did the check on the PlotDataItem.setData() and PlotCurveItem.updateData() instead of the constructors, to cover the possibility of those being called directly. If stepMode is used in the constructors, these methods end up being called anyway.

Note 2: since the warnings module is not otherwise used in these modules, I chose to import it locally so that the whole deprecation check can be cleanly removed once it is no longer needed.

[j9ac9k]

Thanks for pointing this out; this looks good to me.

[j9ac9k]

hey @cpascual thanks for the updates, sorry it's taking me a while to follow up (I'm still unpacking from my move). The changes you made look great; I'm going to toy with it a bit more later, but I do think a deprecation warning should be emitted if stepMode=True is passed into the __init__ methods.

[j9ac9k]

This looks good to me, thanks for the PR @cpascual I'll merge.