Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

time comparisons #18

Closed
stefraynaud opened this issue May 7, 2018 · 15 comments
Closed

time comparisons #18

stefraynaud opened this issue May 7, 2018 · 15 comments
Assignees
Labels
Milestone

Comments

@stefraynaud
Copy link

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
Copy link
Author

stefraynaud commented May 7, 2018

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

@jypeter
Copy link
Member

jypeter commented May 7, 2018

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

@doutriaux1
Copy link
Contributor

@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
Copy link
Author

stefraynaud commented May 7, 2018

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
Copy link
Contributor

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

@doutriaux1
Copy link
Contributor

@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
Copy link
Contributor

dnadeau4 commented May 7, 2018

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

@doutriaux1
Copy link
Contributor

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

@dnadeau4
Copy link
Contributor

dnadeau4 commented May 7, 2018

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
Copy link
Contributor

dnadeau4 commented May 7, 2018

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
Copy link
Contributor

dnadeau4 commented May 7, 2018

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

@dnadeau4
Copy link
Contributor

dnadeau4 commented May 8, 2018

New test pass on OSX and python
#19

@dnadeau4
Copy link
Contributor

dnadeau4 commented May 8, 2018

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
Copy link
Contributor

@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
Copy link
Contributor

CDAT/libcdms#12

@doutriaux1 doutriaux1 added this to the 3.1 milestone Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants