aewallin / allantools

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

Computed Deviation Overwriting bug for set1=at.Dataset(...);set2=at.Dataset(...); set1.compute(); set2.compute() #76

Closed ghost closed 5 years ago

ghost commented 5 years ago

Hi,

we stumbled upon what seems to be an overwriting bug: when creating 2 differents datasets with = at.Dataset(...), and then calling the computing jobs on the 2 datasets with .compute('tdev', ... ), the 2nd computed deviation job overwrites the 1st computed deviation job ie we end up with exactly the same deviation values.

However when doing dataset1= at.Dataset(...) ; dataset1.compute() and then dataset2= at.Dataset(...) ; dataset2.compute() the computed deviation jobs & values don't get overwriten each other.

This behaviors could be reproduced with: AllanTools 2018.03, Python3, Ubuntu 64 bits, and AllanTools 2018.11, Python3, Windows 7 64 bits

Greetings from LNE-Syrte, Paris Florian, Mikkel

aewallin commented 5 years ago

Thanks for finding and reporting this! A minimal example (I think) is something like: ` import allantools as at x1 = at.noise.white(num_points=1024) x2 = at.noise.pink(N=1024)

produces wrong results:

ds1 = at.Dataset(x1) ds2 = at.Dataset(x2) r1 = ds1.compute('oadev') r2 = ds2.compute('oadev') print "r1",r1['stat'][0] print "r1",r2['stat'][0]

produces correct results:

ds1 = at.Dataset(x1) r1 = ds1.compute('oadev') ds2 = at.Dataset(x2) r2 = ds2.compute('oadev') print "r1",r1['stat'][0] print "r2",r2['stat'][0] `

As far as I can tell this is because the inp and out variables are defined after class, but before init over here: https://github.com/aewallin/allantools/blob/master/allantools/dataset.py#L52 That will make them "static", i.e. they belong to the class, not an individual object/instance of the class.

@fmeynadier Ping! is there any design-idea here or is it just a mistake? Ofcourse you might save memory if you don't store big time-series in each Dataset, but static data leads to this surprising/unwanted behaviour...

To make the variables belong to the instance/object they need to be defined as self.foo, under init

ghost commented 5 years ago

Hello Anders, Happy to contribute a little as we are massive users of AllanTools :) Florian

Le 11/12/2018 à 20:57, Anders Wallin a écrit :

Thanks for finding and reporting this! A minimal example (I think) is something like: `import allantools as at

x1 = at.noise.white(num_points=1024) x2 = at.noise.pink(N=1024)

produces wrong results:

ds1 = at.Dataset(x1) ds2 = at.Dataset(x2) r1 = ds1.compute('oadev') r2 = ds2.compute('oadev') print "r1",r1['stat'][0] print "r1",r2['stat'][0]

produces correct results:

ds1 = at.Dataset(x1) r1 = ds1.compute('oadev') ds2 = at.Dataset(x2) r2 = ds2.compute('oadev') print "r1",r1['stat'][0] print "r2",r2['stat'][0] `

As far as I can tell this is because the inp and out variables are defined after class, but before init over here: https://github.com/aewallin/allantools/blob/master/allantools/dataset.py#L52 That will make them "static", i.e. they belong to the class, not an individual object/instance of the class.

@fmeynadier https://github.com/fmeynadier Ping! is there any design-idea here or is it just a mistake? Ofcourse you might save memory if you don't store big time-series in each Dataset, but static data leads to this surprising/unwanted behaviour...

To make the variables belong to the instance/object they need to be defined as self.foo, under init

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/aewallin/allantools/issues/76#issuecomment-446340467, or mute the thread https://github.com/notifications/unsubscribe-auth/AmhmMqg1A8quIDNL6D91zwK2bOZjx_kfks5u4A4egaJpZM4ZN6zo.

fmeynadier commented 5 years ago

Hi everyone,

Definitely a blunder from my side, sorry ! Thanks for pointing it out. I filed a pull request that corrects this behavior, and added the example provided by Anders as an additional test.

aewallin commented 5 years ago

433298a should fix this.

will close this issue if nobody reports more problems within a week or two.

a new release on PyPi might be a good idea - early January 2019?

aewallin commented 5 years ago

closing. please reopen if this is still a problem!