Missing case in concatenate_reversals?

[jockeeson]

Is there a missing case in the concatenate_reversals in rainflow.py file? What happens when t1=0? Or should that be an impossible case?

R1, R2 = reversals1, reversals2
dRstart, dRend, dRjoin = R2[1] - R2[0], R1[-1] - R1[-2], R2[0] - R1[-1]
t1, t2 = dRend*dRstart, dRend*dRjoin
if (t1 > 0) and (t2 < 0):
    result = (R1, R2)
elif (t1 > 0) and (t2 >= 0):
    result = (R1[:-1], R2[1:])
elif (t1 < 0) and (t2 >= 0):
    result = (R1, R2[1:])
elif (t1 < 0) and (t2 < 0):
    result = (R1[:-1], R2)
return np.concatenate(result)

[Gunnstein]

Hey @jockeeson,

You are correct, the case t1=0 is missing.

If the input to concatenate_reversals are in fact reversals, t1=0 is an impossible case since dRstart and dRend cannot be zero.

In practice, the input to concatenate_reversals is the output from find_rainflow_cycles (residue), and looking at the algorithm I am unsure if it is (theoretically) possible that the residue are not reversals.

Since the case t1=0 is not defined, concatenate_reversals would throw a UnboundLocalError in its current implementation when t1=0 since result is defined within the if structure and then referenced in the return np.concatenate(result), try the following to provoke the case and error (dRend=0 -> t1=0):

residue = np.array([1., 2., 0., 3., 4., 1., 1.])

r2 = fatpack.concatenate_reversals(residue, residue)

I have never encountered this case/error and given the amount of use I have put this package/function through I am confident that this is a very rare case, if not impossible.

Perhaps the best way to handle this is to add the case to the if structure and throw a ValueError with a descriptive text indicating that the input must be reversals? Something along the lines of

...
elif (t1 <0) and (t2 < 0):
    result (R1[:-1], R2)
elif (t1==0):
    raise ValueError("Input must be reversals.")

What do you think? Perhaps you could implement it and make a pull request?

Best regards, Gunnstein

[Gunnstein]

Version 0.7.3 adressed this issue and case t1==0 now raises a ValueError.