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

cycling: last day of the month - 31T00 #2382

Open
oliver-sanders opened this issue Aug 8, 2017 · 10 comments
Open

cycling: last day of the month - 31T00 #2382

oliver-sanders opened this issue Aug 8, 2017 · 10 comments
Assignees
Labels
bug Something is wrong :(
Milestone

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Aug 8, 2017

One might expect the sequence 31T00 to yield...

YYYY-01-31T00:00
YYYY-02-28T00:00
YYYY-03-31T00:00
YYYY-04-30T00:00
...

...whereas in actuality it produces:

YYYY-01-31T00:00
YYYY-02-28T00:00
YYYY-03-28T00:00
YYYY-04-28T00:00
...

Strangely 0231T00 yields nothing and more strangely still 0331T00 results in YYYY-03-02T00:00.

This seems a little inconsistent.

@oliver-sanders oliver-sanders added the bug? Not sure if this is a bug or not label Aug 8, 2017
@matthewrmshin matthewrmshin added this to the soon milestone Aug 8, 2017
@oliver-sanders oliver-sanders added bug Something is wrong :( and removed bug? Not sure if this is a bug or not labels Aug 8, 2017
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Aug 8, 2017

03-31T00 resulting in 03-02T00 is definitely worthy of the "bug" label, incidentally R1/20000331T00 works fine!

@matthewrmshin
Copy link
Contributor

🐛 it is.

@arjclark
Copy link
Contributor

arjclark commented Aug 9, 2017

Note that R1/0331T00 doesn't work, suggesting that the problem may be down to partial date expansion.

@dvalters dvalters self-assigned this Aug 9, 2017
@dvalters
Copy link
Contributor

dvalters commented Aug 9, 2017

R1/0329T00 works fine. It seems not to like any combination of month with 30th or 31st day. (Except January...)

(Incidentally, is it supposed to be leap year aware? R1/010229T00 expands to a non-existent date 2001-02-29)

@arjclark
Copy link
Contributor

arjclark commented Aug 9, 2017

Incidentally, is it supposed to be leap year aware? R1/010229T00 expands to a non-existent date 2001-02-29

I think, in practice, that should either never be run or be interpreted as 2001-03-01 (not clear to me which)

@oliver-sanders oliver-sanders self-assigned this Aug 17, 2017
@oliver-sanders oliver-sanders added non-cylc bug This is a bug, but not in Cylc and removed bug Something is wrong :( labels Aug 22, 2017
@oliver-sanders
Copy link
Member Author

Bug is in the isodatetime library, see metomi/isodatetime#80.

@dvalters dvalters removed their assignment Oct 18, 2017
@oliver-sanders oliver-sanders removed their assignment Jul 27, 2018
@matthewrmshin matthewrmshin modified the milestones: soon, cylc-9 Aug 28, 2019
@MetRonnie
Copy link
Member

MetRonnie commented Jul 20, 2023

It is not clear to me how cylc is going from 2001-01-31 to 2001-02-28 since in isodatetime:

2001-01-31 + ---31 = 2001-01-31
2001-02-01 + ---31 = 2001-03-31

@MetRonnie
Copy link
Member

MetRonnie commented Jul 25, 2023

It is not caused by metomi/isodatetime#80 as I have come up with a fix for that and the bug persists here.

Btw I think the sequence 31T00 should only yield the months which have 31 days

2000-01-31
2000-03-31
2000-05-31
...

Whereas it is perfectly possible to use 01T00-P1D, however this is still victim to the bug

2000-01-31
2000-02-29
2000-03-29
...
2001-01-29
2001-02-28
2001-03-28
...

@MetRonnie MetRonnie added bug Something is wrong :( and removed non-cylc bug This is a bug, but not in Cylc labels Jul 25, 2023
@MetRonnie MetRonnie self-assigned this Jul 26, 2023
@MetRonnie
Copy link
Member

MetRonnie commented Jul 26, 2023

The bug is here - the interval is naively taken from the next unit up from the largest truncated property, so 31T00 results in an interval of P1M, and 2000-01-31 + P1M = 2000-02-29

def _get_interval_from_expression(
self, expr: Optional[str], context: Optional['TimePoint'] = None
) -> Optional[Duration]:
if expr is None:
if context is None or not context.truncated:
return None
prop_name = context.get_largest_truncated_property_name()
kwargs = {}
if prop_name == "year_of_century":
kwargs = {"years": 100}
if prop_name == "year_of_decade":
kwargs = {"years": 10}
if prop_name in ["month_of_year", "week_of_year", "day_of_year"]:
kwargs = {"years": 1}
if prop_name == "day_of_month":
kwargs = {"months": 1}

@arjclark
Copy link
Contributor

@MetRonnie - well done tracking that one down

@MetRonnie MetRonnie modified the milestones: cylc-9, cylc-8.4.0 Mar 25, 2024
@oliver-sanders oliver-sanders modified the milestones: 8.4.0, 8.x Jul 9, 2024
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

No branches or pull requests

5 participants