search

iamlikeme / rainflow

Implementation of the rainflow-counting algorythm in Python
MIT License
105 stars 34 forks source link

Remove optional args "left" and "right" and change default behavior to left/right=True #35

Closed iamlikeme closed 4 years ago

iamlikeme commented 4 years ago

Until now, the default behavior of the extract_cycles function was to treat the first and the last point in the time series as non-reversals. The rationale was that in these points it is not possible to determine whether the gradient has changed sign. However, this behavior turned out to be unexpected for users, as attested by issues #4, #9, #29. Some other issues often and even many unit tests use non-default behavior (left=True and right=True).

Therefore, it seems reasonable to change the default behavior, such that the first and the last point in the time series are treated as reversals. It also seems unnecessary to keep the left and right arguments around. If needed, the effect of left/right=False can be achieved simply by dropping the first and the last point from the time series.

iamlikeme commented 4 years ago

@oysteoh @jfcorbett @gsokoll @addarnr @SaetreS Any comments on this PR?

gsokoll commented 4 years ago

Seems reasonable to me. Although this strictly speaking means the algorithm is a reservoir method rather than rainflow method, the practical difference for real world applications (where the data sets are likely to be large) should be negligble.

jfcorbett commented 4 years ago

My comments:

  1. Not being a structural engineer, I won't comment on whether this change is technically appropriate or not, but I can observe that it now makes rainflow's behaviours consistent with MATLAB's own rainflow implementation. So that's certainly a good sign. (Not that I'm a MATLAB fan or anything!)

  2. This is a breaking change. So I would definitely recommend bumping the major version number.

  3. I concur with @gsokoll that this change will have no material impact on the numerical results for real-world applications, so nothing to worry about there.

TL;DR: Looks good to me, as long as the major version gets bumped bc breaking change.

P.S. The parameter names left and right had always irked me (nothing to do with chirality; first and last would have seemed more appropriate), so I'm glad to see them go :-)

iamlikeme commented 4 years ago

Then it is settled. The change will go into version 3.x.