search

iamlikeme / rainflow

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

Added optional arguments nbins and binsize #22

Closed gsokoll closed 4 years ago

gsokoll commented 5 years ago

Added options "nbins" and "binsize" to allow specifying the number or size of cycle-counting bins. These are alternatives to the existing "ndigits" argument. Specifying the bin size or number of bins directly can be useful when undertaking fatigue analysis of multiple datasets, where consistency of number of bins or bin size is desired.

iamlikeme commented 5 years ago

Hey, thanks for taking your time to contribute to this repository.

I think binning can be easily accomplished using numpy, for example:

series = [0, -2, 1, -3, 5, -1, 3, -4, 4, -2, 0]
cycles = rainflow.count_cycles(series, left=True, right=True)

ranges = [delta for delta, count in cycles]
counts = [count for delta, count in cycles]
np.histogram(ranges, weights=counts, bins=2, range=(0, 9))
# output: (array([3., 2.]), array([0. , 4.5, 9. ]))

Is there a good reason to implement this functionality in rainflow, rather that just using numpy?

gsokoll commented 5 years ago

No problems Piotr. I did originally look at using np.histogram, but decided to implement the binning within the rainflow command for a few reasons:

It's your choice whether you think these are good enough reasons. The proposed changes don't take anything away from the current functionality.

uzi0espil commented 5 years ago

First of all, thanks for the great package. I just wanted to say that I agree with @gsokoll, I think it is important. I was about to fork and implement the bin and binsize. So thank you for the contributions.

iamlikeme commented 5 years ago

Ok, let's go ahead with it then.

One thing that surprised me in the proposed implementation is that it returns left edges of the bins. For example:

series = [0, -2, 1, -3, 5, -1, 3, -4, 4, -2, 0]
rainflow.count_cycles(series, left=True, right=True, bins=2)
# output: [(0.0, 3.0), (4.5, 2.0)]

I would normally expect to get the right bin edges: [(4.5, 3.0), (9.0, 2.0)]. This would give a conservative fatigue life estimate.

Do you have an opinion about it @gsokoll @uzi0espil ? What are the conventions used in other software?

gsokoll commented 5 years ago

It's probably fair to say that there is no well-established convention for definition of the bin edges.

I have no strong preference, although I dislike the mean value. Using either high or low edge it is easy for the user to take into account the width of the bins when calculating actual fatigue damage. Perhaps using high edge would be a safe default position, for users who do not stop to think about the issue.

Whatever choice is made, it needs to be very clear and obvious to the user which bin edges are returned !

iamlikeme commented 5 years ago

Thanks for the excellent review of binning conventions. Since there is no well established convention I suggest that we go for the right bin edge (should be mentioned in the docstring) and we add an option for left edge or center in a followup pull request.

gsokoll commented 5 years ago

I've changed the bin definition to now refer to the right-hand edge.

Travis continuous integration is reporting that "some checks haven't completed yet" - not sure entirely what that is, but there is some discussion here. I don't think I can fix this wihout having write access to the repo. https://travis-ci.community/t/known-issue-travis-ci-reports-expected-waiting-for-status-to-be-reported-on-the-github-status-api-but-the-status-never-arrives/1154

iamlikeme commented 5 years ago

Hey @gsokoll, it looks like the pull request is almost ready, but you still have to correct the test cases (test_rainflow_nbins and test_rainflow_binsize). The tests expect that bins are defined by their left edge but the rainflow.count_cycles returns right edges (which is the correct behavior).

gsokoll commented 4 years ago

Sorry for the delay in getting back to this. Test case results have been updated to use right-hand side bins. Seems to pass Travis CI now. Hopefully this can be pulled into the master branch now.

iamlikeme commented 4 years ago

Published on PyPI as v2.2.0. Thank you for your contribution @gsokoll :)