aewallin / allantools

Allan deviation and related time & frequency statistics library in Python
GNU Lesser General Public License v3.0
222 stars 76 forks source link

taus='all' doesn't generate all #142

Closed zpillio closed 1 year ago

zpillio commented 1 year ago

When taus='all' option is used some taus are missing. It happens because 'np.floor' used here:

https://github.com/aewallin/allantools/blob/8f1ed9dbeab61859d909839da22ec646b1e3aa0f/allantools/allantools.py#L1713

In taus='all' case 'taus' are calculated from integer window sizes, and calculating back the window sizes and using 'floor' can produce duplication which is filterred later out.

Consider changing it to 'np.round' which reproduces the original window size and solves this issue.

aewallin commented 1 year ago

with a one-line change I get failures of test_ns.py e.g.: https://github.com/aewallin/allantools/actions/runs/4868282641/jobs/8681632645?pr=143

I may investigate this later - but comments/pull-requests are very welcome..

zpillio commented 1 year ago

with a one-line change I get failures of test_ns.py e.g.: https://github.com/aewallin/allantools/actions/runs/4868282641/jobs/8681632645?pr=143

I may investigate this later - but comments/pull-requests are very welcome..

I think it has similar numerical reason, because the window range is validated by comparing floating points here which doesn't take account the case when it's rounded up.

https://github.com/aewallin/allantools/blob/8f1ed9dbeab61859d909839da22ec646b1e3aa0f/allantools/allantools.py#L1711

Changing the validation to be done on the rounded window sizes makes the test pass.

I can't push my branch, because: "Permission to aewallin/allantools.git denied to zpillio." It may an issue on my side, but I'm not using github actively, and I haven't figured out, but here is a diff:

diff --git a/allantools/allantools.py b/allantools/allantools.py
index 3c90478..7ca84f2 100644
--- a/allantools/allantools.py
+++ b/allantools/allantools.py
@@ -1706,11 +1706,12 @@ def tau_generator(data, rate, taus=None, v=False, even=False, maximum_m=-1):
     # mtotdev   2
     # ttotdev   2

-    taus_valid1 = taus < (1 / float(rate)) * float(len(data))
-    taus_valid2 = taus > 0
-    taus_valid3 = taus <= (1 / float(rate)) * float(maximum_m)
+    m = np.round(taus * rate)
+    taus_valid1 = m < len(data)
+    taus_valid2 = m > 0
+    taus_valid3 = m <= maximum_m
     taus_valid = taus_valid1 & taus_valid2 & taus_valid3
-    m = np.floor(taus[taus_valid] * rate)
+    m = m[taus_valid]
     m = m[m != 0]       # m is tau in units of datapoints
     m = np.unique(m)    # remove duplicates and sort
fmeynadier commented 1 year ago

Thanks for the bug report and the diff !

If you want to appear as a contributor, the usual workflow on github is to fork the "allantools" repo under your own account, then perform the modifications on your fork, then issue a "pull request" (see https://docs.github.com/fr/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request)

Otherwise one of us will do it...

aewallin commented 1 year ago

closed via #144 (?)