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

final cycle point offset giving wrong result #4908

Closed
oliver-sanders opened this issue Jun 9, 2022 · 7 comments · Fixed by #5031
Closed

final cycle point offset giving wrong result #4908

oliver-sanders opened this issue Jun 9, 2022 · 7 comments · Fixed by #5031
Assignees
Labels
bug Something is wrong :(
Milestone

Comments

@oliver-sanders
Copy link
Member

Note: Reported with Cylc 7, replicated with Cylc 8.

In this example the final cycle point ends up on the 30th of December rather than the 31st as expected:

[scheduling]
    cycling mode        = gregorian
    initial cycle point = 20210701T0000Z
    final cycle point   = +P6M-P1D
    [[dependencies]]
        [[[R1/^]]]
            graph = start    # 2021-07-01
        [[[R1/$]]]
            graph = stop     # 2021-12-30

This is surprising given isodatetime yields the 31st:

$ rose date 20210701T0000Z --offset '+P6M' --offset '-P1D'  # rose 2019
20211231T0000Z
$ isodatetime 20210701T0000Z --offset '+P6M' --offset '-P1D'
20211231T0000Z

Swap months for years and the calculation comes out correctly:

[scheduling]                                                                         
    cycling mode        = gregorian        
    initial cycle point = 2000             
    final cycle point = +P1Y-P1D           
    [[dependencies]]                       
        [[[R1/^]]]                         
            graph = start  # 2000-01-01    
        [[[R1/$]]]  
            graph = stop   # 2000-12-31  

Pull requests welcome!
This is an Open Source project - please consider contributing a bug fix
yourself (please read CONTRIBUTING.md before starting any work though).

@oliver-sanders oliver-sanders added the bug Something is wrong :( label Jun 9, 2022
@oliver-sanders oliver-sanders added this to the cylc-8.x milestone Jun 9, 2022
@MetRonnie
Copy link
Member

MetRonnie commented Jun 9, 2022

I think this is an isodatetime bug:

>>> from metomi.isodatetime.data import *
>>> a = TimePoint(year=2021, month_of_year=7)
2021-07-01T00Z
>>> b = Duration(months=6, days=-1)
P6M-1D
>>> a + b
2021-12-30T00Z

Note:

>>> a + Duration(months=6) - Duration(days=1)
2021-12-31T00Z  # correct answer

@oliver-sanders oliver-sanders added bug? Not sure if this is a bug or not non-cylc bug This is a bug, but not in Cylc bug Something is wrong :( and removed bug Something is wrong :( bug? Not sure if this is a bug or not non-cylc bug This is a bug, but not in Cylc labels Jun 9, 2022
@oliver-sanders
Copy link
Member Author

I've flip-flopped on this one, isodatetime is arguably doing the right thing, Cylc just needs to handle the two durations independently rather than merging them together.

@MetRonnie
Copy link
Member

But surely in isodatetime

a + Duration(months=6) - Duration(days=1)

should give the same result as

a + Duration(months=6, days=-1)

?

@MetRonnie
Copy link
Member

MetRonnie commented Jun 9, 2022

Actually in the case of a + Duration(months=6, days=-1) I think it might be doing

(a - Duration(days=1)) + Duration(months=6)

which would explain the answer of 2021-12-30.

Edit: yes

2021-03-01 + P1M-1D gives 2021-03-28

@oliver-sanders
Copy link
Member Author

But surely in isodatetime
...
should give the same result as

I'm not sure about that, you could argue both ways. You're asking isodatetime to subtract one day from a month.

@MetRonnie
Copy link
Member

MetRonnie commented Jun 9, 2022

Ok, ignore the subtraction, this problem still occurs for two additions:

>>> 2021-02-28 + P1M + P1D
2021-03-29
>>> 2021-02-28 + P1M1D
2021-04-01

(I've used shorthand to represent the metomi.isodatetime.data objects)

The problem seems to be isodatetime applies the additions in order of smallest unit to largest, which is opposite to their representation (P[largest->smallest]) and so opposite to the expected order of mathematical operations.

@MetRonnie
Copy link
Member

MetRonnie commented Jun 9, 2022

I have debugged a workflow with

    initial cycle point = 2021-02-28
    final cycle point = +P1M+P1D

and found that Cylc does indeed convert the expression +P1M+P1D into the isodatetime Duration P1M1D, in

def _get_point_from_expression(

However it also converts the reverse +P1D+P1M into the same P1M1D. So it looks like there are two bugs:

  • isodatetime should probably do addition of a conjugate Duration in order of largest unit to smallest
  • cylc-flow should not convert expressions that add durations into a single conjugate duration, they should be kept separate

@MetRonnie MetRonnie modified the milestones: cylc-8.x, cylc-8.0rc5 Jun 10, 2022
@MetRonnie MetRonnie self-assigned this Jun 10, 2022
@MetRonnie MetRonnie mentioned this issue Aug 1, 2022
8 tasks
oliver-sanders pushed a commit to oliver-sanders/cylc-flow that referenced this issue Aug 15, 2022
Fix conjugate duration bug

* Closes cylc#4908
* Fix bug where specifying multiple datetime offsets would not obey the given order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants