Invert order of checks when time ticks

[kinow]

Hi @oliver-sanders

I was browsing issues when I found #80 reported via a Cylc issue. I was curious to see in which part of the code it was happening. But then after debugging it once, it occurred to me that a possible solution would be to invert the order of the checks (i.e. the if/else's) when the time is "ticked" and corrected when adding truncated TimePoints.

But... even though I wrote a quick test and the possible fix, I am still not confident that that's the solution. Would you mind have a look at this one whenever you have time, please?

I am assuming that different than

2000-01-01T00:00:00Z + 03-30T00:00:00Z = 2000-03-02T00:00:00Z

the expected result was actually

2000-01-01T00:00:00Z + 03-30T00:00:00Z = 2000-03-30T00:00:00Z

If this assumption is wrong, then you probably don't even have to spend time looking at the code/test 😬

Cheers
Bruno

ps: in the Cylc ticket you mentioned other cases where the result was strange... I haven't tested those scenarios... maybe it could fix that too, or there could be other parts that need some fix...

[matthewrmshin]

@benfitzpatrick do you mind commenting on this one?

[benfitzpatrick]

Thank you! Really glad the bug is fixed - but I suspect there might be another bug revealed by one of the tests in my comments...

[benfitzpatrick]

I definitely agree there is a bug in the current implementation.

The main idea is that the timepoint is incremented until the data matches the truncated data... so the properties specified in the truncated timepoint should equal the ones in the final timepoint (assuming they are self-consistent).

[kinow]

I think I now understand a bit better how the previous code worked @benfitzpatrick , so reverted my change, leaving only the tests. Will need some more time, but I think it might be possible to find a simpler fix.

This fix should make all the three tests in this current pull request to work. Once that's done, I will add a few more tests, and if no other bugs appear, then it will be ready for another review 👍

Thanks!
Bruno

[kinow]

I think I know one way to fix it, but it would take a while to implement. It would change the order of checks to go from the largest to smallest (easy to implement), but then in data.py's TimePoint.tick_over() I would have to change the logic to also match this behaviour. Yesterday when flying from Wellington back to Auckland I did a few tests on a piece of paper, and it apparently works for both the current bug, and also for the new tests added by @benfitzpatrick

Will update the pull request later. First will focus on Python 3 and packaging 👍

Cheers
Bruno

[benfitzpatrick]

Thanks very much!

[lhuggett]

This is also an issue with ordinal date when going from leap-year to non-leap-year (and vice-versa), and week/day of week, since the ordinal date should be found in the destination year, not the initial year. The issue should be fixed by this change, since year needs to be ticked over before day-of-year or week-of-year. I suggest adding a couple of tests for these. Something like:

def get_truncated_timepoint_add_tests():
    """Yield tests for adding a truncated timepoint to a timepoint."""
    return [
        ("2010-06-15T12:29:38Z",
         "-034",
         "2011-02-03T12:29:38Z"),
        ("2010-06-15T12:29:38Z",
         "08354",
         "2108-12-19T12:29:38Z"),
    ]

in the timepoints to test section, and then an extra function

    def test_truncated_timepoint_add(self):
        """Test adding a truncated time point to a time point."""
        parser = parsers.TimePointParser(
            allow_truncated=True)
        for test_props1, test_props2, ctrl_string in (
                get_truncated_timepoint_add_tests()):
            point1 = parser.parse(test_props1)
            point2 = parser.parse(test_props2)
            test_point = (point1 + point2).to_calendar_date()
            test_string = str(test_point)
            self.assertEqual(test_string, ctrl_string,
                             "%s + %s" % (point1, point2))

in the TestSuite class.

[kinow]

@benfitzpatrick spent a couple of hours this morning scratching my head and running tests... trying to find a solution with as little changes as possible...

Found one way through a target object, used to prevent the timepoint + truncated timepoint operation of passing over the limits of what's defined in the truncated timepoint. Implemented it with an optional parameter, so that other parts of the code are not changed.

All tests passing, but wonder if you have any more tests that you can think of, that would be useful for this pull request?

[kinow]

Oh, just saw @lhuggett 's last comment. Thought I had included those tests, but I had not. Just did, and now the build is failing locally. Let me see why that's happening...

[kinow]

@lhuggett I had a look at the tests, and the -034 I believe stands for day_of_year. At least that's where that value was put after the parser processed it.

Debugging further ahead, I noticed that actually the add_truncated function could work with this new test and with the changes in this pull request in theory, as you suggested.

However, the current add_truncated function doesn't appear to support a day_of_year yet. So nothing gets changed for that value, at least from what I could tell debugging the code.

I think it would be simpler to leave it for another issue after this one. What do you think?

[benfitzpatrick]

I think it is tricky doing it this way around. I managed to find a test that won't work - if you leave a 'time-bomb' the date can get messed up:

    def test_timepoint_adding_truncated_timepoint_jan_to_feb(self):
        """Test rolling over the month and day of month."""
        t1 = data.TimePoint(year=2000, month_of_year=1, day_of_month=31,
                            hour_of_day=23, minute_of_hour=0,
                            second_of_minute=0, time_zone_hour=0,
                            time_zone_minute=0)
        t2 = data.TimePoint(month_of_year=2, day_of_month=28,
                            hour_of_day=0, minute_of_hour=20,
                            second_of_minute=0, time_zone_hour=0,
                            time_zone_minute=0, truncated=True)
        t3 = t1 + t2
        self.assertEqual(2000, t3.year)
        self.assertEqual(2, t3.month_of_year)
        self.assertEqual(28, t3.day_of_month)
        self.assertEqual(0, t3.hour_of_day)
        self.assertEqual(20, t3.minute_of_hour)
        self.assertEqual(0, t3.second_of_minute)
        self.assertEqual(data.Duration(hours=0, minutes=0), t3.get_time_zone())
        self.assertEqual("2000-02-28T00:20:00Z", str(t3))

fails with '28 != 29' in the day_of_month ☹️

[kinow]

@benfitzpatrick thanks for the quick feedback. I found where to fix it, and even made the test pass... but then realized the amount of extra work to keep adding this if not target ..., and the complexity of methods (which are already complex enough) would increase a lot.

I think I will discard this approach of inverting order of checks + target, go back to the whiteboard and see what other options we would have. 👍

(but will add you last test to the test suite too, thanks!)

[kinow]

Will close this one as there are conflicts, and I never managed to solve the issues in the tests.

Some other day will review the ticket and try again. I will just add a note to the ticket about the interesting comments and possible cases for unit tests that are in this pull request, so that either myself or any other developer can have some background when working on this issue.