aewallin/allantools

taus='all' doesn't generate all

zpillio opened this issue · 4 comments

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

m = np.floor(taus[taus_valid] * rate)

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.

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..

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.

taus_valid3 = taus <= (1 / float(rate)) * float(maximum_m)

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

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...

closed via #144 (?)