CDAT / cdtime

climate calendar manipulation tools
0 stars 2 forks source link

time comparisons #18

Closed stefraynaud closed 6 years ago

stefraynaud commented 6 years ago

Hi, time comparisons seem now buggy. Here are various tests:

In [5]: cdtime.reltime(0,'hours since 2000')>cdtime.reltime(1,'hours since 2000')
Out[5]: False

In [6]: cdtime.reltime(0,'hours since 2000')<cdtime.reltime(1,'hours since 2000')
Out[6]: True

In [7]: cdtime.comptime(2000)>cdtime.comptime(2001)
Out[7]: False

In [8]: cdtime.comptime(2000)<cdtime.comptime(2001)
Out[8]: False

In [9]: cdtime.reltime(0,'years since 2000')>cdtime.reltime(1,'years since 2000')
Out[9]: False

In [10]: cdtime.reltime(0,'years since 2000')<cdtime.reltime(1,'years since 2000')
Out[10]: False

In [11]: cdtime.comptime(2000,1,1,0)>cdtime.comptime(2000,1,1,1)
Out[11]: True

In [12]: cdtime.comptime(2000,1,1,0)<cdtime.comptime(2000,1,1,1)
Out[12]: True

What changed?

stefraynaud commented 6 years ago

I think it is linked to bug #8 This breaks alot of code still running under python2.

jypeter commented 6 years ago

I hope nothing is broken in the python2 version! I have not installed v8 yet

doutriaux1 commented 6 years ago

@stefraynaud yes I think @dnadeau4 broke this while porting to 3.0, please use cmp function on the objects. @dnadeau4 any chance to re-enable this? Can we at least make it fail rather than giving wrong answers?

stefraynaud commented 6 years ago

It's almost impossible to identify all cdtime comparisons in tens of thousands of code lines. It should be as difficult as to convert everybody quickly to python3. :)

doutriaux1 commented 6 years ago

@stefraynaud agreed! Let's wait and hear @dnadeau4 reasoning on why this is gone.

doutriaux1 commented 6 years ago

@stefraynaud yes you need to use the method cmp attached on objects, sorry my original post was misleading. @dnadeau4 it looks like this needs to be fixed.

dnadeau4 commented 6 years ago

@doutriaux1 no way to re-enable this. "cmp" no longer exist in python3. All tests pass, maybe you could write better tests.

doutriaux1 commented 6 years ago

@dnadeau4 maybe i could you're right. But now @stefraynaud gave you good tests, so thanks.

dnadeau4 commented 6 years ago

It might be a bug in python. I am not sure what is going on with python2. I cannot bring the overload into the richcompare() function in the debugger. Maybe the python 2 module is not setup right.


python2
In [19]: cdtime.comptime(2000)<cdtime.comptime(2001)
Out[19]: False

In [20]: cdtime.comptime(2000)<cdtime.comptime(2001)
Out[20]: True

In [21]: cdtime.comptime(2000)<cdtime.comptime(2001)
Out[21]: False

In [22]: cdtime.comptime(2000)<cdtime.comptime(2001)
Out[22]: True

In [23]: cdtime.comptime(2000)<cdtime.comptime(2001)
Out[23]: False

In [24]: aa = cdtime.comptime(2000)

In [25]: bb = cdtime.comptime(2001)
    ...: 

In [26]: aa < bb
Out[26]: True

In [27]: aa < bb
Out[27]: True

In [28]: aa < bb
Out[28]: True

In [29]: aa < bb
Out[29]: True
dnadeau4 commented 6 years ago

cmp which is python2 ways to do compare works.

In [33]: cdtime.comptime(2000).cmp(cdtime.comptime(2001))
Out[33]: -1

In [34]: cdtime.comptime(2001).cmp(cdtime.comptime(2000))
Out[34]: 1
dnadeau4 commented 6 years ago

Not sure if that works on the MAC, need testing... 432ddb9faab79d6a5a6863b9758c09218152090e

dnadeau4 commented 6 years ago

New test pass on OSX and python https://github.com/CDAT/cdtime/pull/19

dnadeau4 commented 6 years ago

I had to set 2 different comparisons tp_compare for python 2 and tp_richcompare for python 3.

Python 2 calls tp_compare when using <, >,=,.. operators.
Python 3 calls tp_richcompare.

Actually,

Python 2 will call tp_richcompare if you use __lt__, __eq__, ..., but will call tp_compare if you use <, ==.

So a<b is a different funtion call than a.__lt__(b) in python 2, but the same function call in python 3.

dnadeau4 commented 6 years ago

@stefraynaud Your issue has been solved and merged to master. Although @doutriaux1 test found conversion issues in libcdms. I have not pin point the source of that error yet. The year pass is YYYY-XX :wow:

dnadeau4 commented 6 years ago

https://github.com/CDAT/libcdms/issues/12