Optionally include open cycles in rainflow cycle counting

[adtzlr]

by adding a new function argument find_rainflow_cycles(reversals, include_open_cycles=False).

This is a follow-up of #17 for the suggestion originally posted by @Gunnstein in #17 (comment).

Description

Move the (duplicated) code-blocks for the first- and second-pass from find_rainflow_ranges() and find_rainflow_ranges_strict() to find_rainflow_cycles(include_open_cycles=False).

Originally posted by @adtzlr in #17 (comment)

Solution

I added an optional recursive call to find_rainflow_cycles(reversals). #19 (comment)

✅ Backward compatible
✅ Tests

[adtzlr]

@Gunnstein as this is only a minor change compared to #17, I think it is way easier to decide for you if this change can be merged or not.

Should I apply the changes to the Notebook again?

[adtzlr]

However, I'm not 100% sure about the residuals: I did not change the residue for find_rainflow_cycles(reversals, include_open_cycles=True), i.e. the residuals are always the ones from the first pass (because they are empty for the second pass). Do you think they should be overwritten to an empty list instead?

Right now my implementation is consistent with the (unmodified) docstring:

def find_rainflow_cycles(reversals, include_open_cycles=False):
    """
    ...
    residue : 1darray
        The residue of the reversal series after one pass of the rainflow
        algorithm.
    ...
    """

[adtzlr]

Here is the recursive function call.

[Gunnstein]

Dear @adtzlr,

Thank you again for your efforts.

Regarding the residual

However, I'm not 100% sure about the residuals: I did not change the residue for find_rainflow_cycles(reversals, include_open_cycles=True), i.e. the residuals are always the ones from the first pass (because they are empty for the second pass). Do you think they should be overwritten to an empty list instead?

The residual after the first pass and the residual of the concatenated residuals (second pass) are the same:

So in one sense, the residual is an invariant of a sequence of reversals (or even signal if k is large enough). It does not matter if the residual of the first or second pass is returned, the residual of a cycle count is the residual.

On the other hand it may be a bit confusing to have a residual after extracting "all cycles", including the open ones. And, for the majority of use cases, the residual will not be used/needed if the cycles already include the open ones, i.e. if the residual is processed. For the rare cases in which the residual is needed, it is still fully possible and equally efficient to extract the residual.

Looking at it now, since the cases where a residual is actually going to be used are the exceptions (optional ones); It would have been more natural (and produced cleaner code) to have the function return all cycles by default (include_open_cycles=True), and only return the cycles (cycles = find_rainflow_cycles(reversals)). This would avoid statements like:

cycles, _= find_rainflow_cycles(reversals, include_open_cycles=True)

Unfortunately, this breaks backward compatibility, and all existing code out there that uses find_rainflow_cycles would have to be modified.

Ultimately, I think it is ok if the function always returns both cycles and residual, even if the residual is very rarely being used. (I dont know of any downside in terms of performance?) I would then change the documentation to

residue : 1darray
    The residue of the reversal series after processing with the rainflow algorithm.

instead of

residue : 1darray
        The residue of the reversal series after one pass of the rainflow algorithm.

Ideally, the Example section for find_rainflow_cycles should also be modified to include the option, for instance by adding

Alternatively, the steps above to extract the rainflow cycles from the residue 
and concatenate them with the cycles from the first pass can be achieved directly 
by the `include_open_cycles` option, i.e.

>>> rainflow_cycles, _ = fatpack.find_rainflow_cycles(rev_1, include_open_cycles=True)

to the very end of the docstring.

Regarding the notebook

Should I apply the changes to the Notebook again?

That would be great!

Best regards,
Gunnstein

[adtzlr]

Hey @Gunnstein,

sorry for not replying for such a long time. Unfortunately, I don't had and have the time in the next week(s) to work on that topic. FYI: Personally I'm using the other (in my opinion much simpler) approach for eliminating the residuals described ISO 12110-2 now, where the signal is splitted and re-combined at its maximum (section A.3.3.3).

def my_find_rainflow_ranges(y, k=64, return_means=False, return_cycles=False):

    from fatpack import find_rainflow_cycles, find_reversals

    idx = np.argmax(y)
    y_rearranged = np.concatenate([y[idx:], y[: idx + 1]])
    reversals = find_reversals(y_rearranged, k)[0]

    cycles, residue = find_rainflow_cycles(reversals)
    cycles = np.vstack([cycles.reshape(-1, 2), [reversals.max(), reversals.min()]])

    ranges = np.abs(cycles[:, 1] - cycles[:, 0])

    if return_means:
        means = 0.5 * (cycles[:, 0] + cycles[:, 1])
        if return_cycles:
            out = ranges, means, cycles
        else:
            out = ranges, means
    else:
        if return_cycles:
            out = ranges, cycles
        else:
            out = ranges
    return out

Thanks for sharing fatpack, great to have such a solid code base available at PyPI!