Improve TimeRecurrence class #183

MetRonnie · 2020-09-10T15:25:54Z

Follow up to #165

Implements an __add__ & __sub__ methods in TimeRecurrence, allowing Durations to be added to TimeRecurrences. This takes the recurrence series and offsets it by the given duration.
Fixes a longstanding bug Fix the implementation of recurrence format number 1 #45 where the implementation of TimeRecurrence format no. 1 was incorrect. This format seems rarely used in Cylc so hopefully we can get away with this breaking change.

The first recurrence format should not define repetitions in between the given start and end date - it actually should just extract the interval between the start and end dates and repeat that interval, starting at the start date

Includes tests
Changelog updated

oliver-sanders

~~Looks ok, a quick test would be nice.~~ (Outdated)

MetRonnie · 2020-09-17T17:38:30Z

Codacy is complaining about

Expression (recurrence_fmt3) + (data.TimePoint(year=2049, month_of_year=2)) is assigned to nothing

in a test. Can we change pylint to flake8 on there, and if so, how?

wxtim · 2020-09-18T08:51:57Z

Can we change pylint to flake8 on there, and if so, how?

I can't see a setup file so I think that you need to get a product owner on Codacy to change things there. I wouldn't worry too much about codacy failures.

I've proposed a change of the testing to use flake8, on the grounds of consistency with other projects at #185

Outdated

wxtim

Essential

works
tests test what they should test

Non-essential

tests could look nicer. TBH I think there is a case for an entirely separate PR to refactor the tests for isodatetime into a pytest format. Don't worry about it here if you don't want to.

wxtim · 2020-10-07T14:08:23Z

metomi/isodatetime/tests/test_00.py

+            repetitions=rep, duration=dur,
+            end_point=data.TimePoint(year=2020, month_of_year=4,
+                                     day_of_month=3))
+        assert recurrence_fmt3 == recurrence_fmt4


Why are you asserting equality here, when this is test for adding?
Is it to establish equality before you operate on these recurrences?
Should you also be asserting that recurrence_fmt3 == recurrence_fmt1

Suggested change

assert recurrence_fmt3 == recurrence_fmt4

# check that all three recurrence formats are seen as equal.

assert recurrence_fmt1 == recurrence_fmt3 == recurrence_fmt4

Yes, I figured as they are meant to be the same it would help to check that in case something breaks (although there is a separate test for the TimeRecurrence comparison operators, at least it should let the developer know it's not a problem with the add/sub methods).

Format 1 might have been missed out because I hadn't yet fixed that format when writing the test, not sure

wxtim · 2020-10-07T14:13:24Z

metomi/isodatetime/tests/test_00.py

+        expected = data.TimeRecurrence(
+            repetitions=rep, start_point=new_start_pt, duration=dur)
+        for recurrence in (recurrence_fmt1, recurrence_fmt3, recurrence_fmt4):
+            assert recurrence + offset == expected


This feels to me like it would be a lot cleaner parameterized. Sorry - hobby horse.

wxtim · 2020-10-07T14:17:21Z

metomi/isodatetime/tests/test_00.py

+        for recurrence in (recurrence_fmt1, recurrence_fmt3, recurrence_fmt4):
+            assert recurrence - offset == expected
+
+        with pytest.raises(TypeError):


IMO you should not be testing for success and failure in the same test as success.

wxtim · 2020-10-07T14:49:43Z

metomi/isodatetime/tests/test_00.py

+        with pytest.raises(TypeError):
+            recurrence_fmt3 + data.TimePoint(year=2049, month_of_year=2)
+
+    def test_invalid_timerecurrence(self):


My view is that this would look nicer as

STUFF = { 'start_pt': data.TimePoint(year=2020, day_of_month=9), 'end_pt': data.TimePoint(year=2020, day_of_month=30), 'dur': data.Duration(hours=36) } @pytest.mark.parametrize( 'kwargs', [ { "repetitions": 4, "start_point": STUFF['start_pt'], "end_point": STUFF['end_pt'], "duration": STUFF['dur'] }, { "repetitions": None, "start_point": STUFF['start_pt'], "end_point": STUFF['end_pt'], "duration": STUFF['dur'] }, ] ) def test_invalid_timerecurrence(kwargs): """Test that init'ing a TimeRecurrence with bad inputs raises error.""" with pytest.raises(BadInputError): data.TimeRecurrence(**kwargs)

Just tried that but it seems you can't parametrize methods of a class that extends unittest.TestCase. At some point we'll overhaul these tests... at some point... But I imagine it makes sense to leave the tests grouped together until such a time

Not too fussed, however, easy enough to dedent and remove the self argument.

oliver-sanders

Looks ok to me:

Could do with a couple of simple str tests since that logic has changed slightly.
Couple of docs changes may require a little thought.

The old behaviour was quite nice for arbitrary "chunking":

R5/2000/2001  # split 2000 into five chunks

So there is a risk it may be in use somewhere. I've done a scan on our site and can't find any active uses so hopefully we are safe on that one. It would be a good idea to come up with an alternative (even if it uses Jinja2) we can document somewhere [in Cylc].

Reminder (to myself or whoever updates isodatetime in Cylc) to run though the recurrence docs (which could do with some TLC), check examples and document this.

README.md

oliver-sanders · 2020-10-08T09:49:40Z

metomi/isodatetime/data.py

+    repetitions (int): The number of repetitions in the recurrence. If set
+        to 1, the recurrence simply consists of one date-time, with no
+        duration (i.e., the number of repetitions is inclusive of the first
+        occurrence). If omitted, the number of repetitions is unbounded.


TODO:

Check that R/2000 and R1/2000 still do the same thing.

Isn't R/2000 invalid, as that means it's unbounded, but then no duration is specified so it's impossible to make a sequence out of it?

Sorry, this was a note to myself. Just checked R/2000 and R1/2000 still do the same thing they did before.

metomi/isodatetime/data.py

Updated/added relevant tests

Plus add TimeRecurrence __add__ test cases for fmt 1

Plus raise exception for a couple of cases of invalid TimeRecurrence inputs

oliver-sanders

Good to go here I think.

Running the cylc test battery against this branch I get the same number of failures as for the master branch which is hopefully a good sign that this is unlikely to have broken anything.

MetRonnie added this to the 2.1 milestone Sep 10, 2020

MetRonnie requested a review from oliver-sanders September 10, 2020 15:25

MetRonnie self-assigned this Sep 10, 2020

oliver-sanders previously approved these changes Sep 11, 2020

View reviewed changes

MetRonnie marked this pull request as draft September 11, 2020 11:02

MetRonnie changed the title ~~Implement _copy() method for TimeRecurrence~~ Implement __add__/__sub__ methods for TimeRecurrence Sep 14, 2020

MetRonnie force-pushed the hashable-data2 branch from f496ee8 to 51da3f5 Compare September 17, 2020 17:24

MetRonnie changed the title ~~Implement __add__/__sub__ methods for TimeRecurrence~~ Improve TimeRecurrence class Sep 17, 2020

MetRonnie marked this pull request as ready for review September 17, 2020 17:34

MetRonnie requested a review from oliver-sanders September 17, 2020 17:35

MetRonnie added the bug label Sep 17, 2020

MetRonnie requested a review from wxtim September 17, 2020 17:38

MetRonnie linked an issue Sep 17, 2020 that may be closed by this pull request

Fix the implementation of recurrence format number 1 #45

Closed

wxtim approved these changes Oct 8, 2020

View reviewed changes

oliver-sanders reviewed Oct 8, 2020

View reviewed changes

metomi/isodatetime/data.py Show resolved Hide resolved

MetRonnie force-pushed the hashable-data2 branch from 51da3f5 to a8473fc Compare October 8, 2020 12:22

MetRonnie added 9 commits October 8, 2020 14:26

Implement __add/sub__ methods for TimeRecurrence

9ed4905

Improve TimeRecurrence handling of edge cases

4282440

Improve TimeRecurrence documentation

89224d8

Preserve TimeRecurrence format when adding Durations to one

a4794d0

Fix recurrence format no. 1 (issue metomi#45)

c6371c7

Updated/added relevant tests

Fix Duration.__add__(TimeRecurrence)

404aca0

Plus add TimeRecurrence __add__ test cases for fmt 1

Add more TimeRecurrence tests

045e150

Plus raise exception for a couple of cases of invalid TimeRecurrence inputs

Clarify docs on recurrence fmt 1; also update changelog

cc470b7

Fix test post-rebase & turn off verbose mode in Travis flake8

71b6e6d

MetRonnie force-pushed the hashable-data2 branch from a8473fc to 71b6e6d Compare October 8, 2020 13:29

oliver-sanders approved these changes Oct 15, 2020

View reviewed changes

oliver-sanders merged commit 0a0f20c into metomi:master Oct 15, 2020

MetRonnie deleted the hashable-data2 branch October 15, 2020 14:54

MetRonnie mentioned this pull request Jul 28, 2021

Update isodatetime cylc/cylc-flow#4110

Closed

MetRonnie mentioned this pull request Mar 30, 2022

Change next release from 2.1 to 3.0 #205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve TimeRecurrence class #183

Improve TimeRecurrence class #183

MetRonnie commented Sep 10, 2020 •

edited

Loading

oliver-sanders left a comment •

edited by MetRonnie

Loading

MetRonnie commented Sep 17, 2020

wxtim commented Sep 18, 2020 •

edited

Loading

wxtim left a comment

wxtim Oct 7, 2020

MetRonnie Oct 8, 2020

wxtim Oct 7, 2020

wxtim Oct 7, 2020

MetRonnie Oct 8, 2020

wxtim Oct 7, 2020

MetRonnie Oct 8, 2020

oliver-sanders Oct 8, 2020

oliver-sanders left a comment •

edited

Loading

oliver-sanders Oct 8, 2020

MetRonnie Oct 8, 2020

oliver-sanders Oct 15, 2020

oliver-sanders left a comment

	assert recurrence_fmt3 == recurrence_fmt4
	# check that all three recurrence formats are seen as equal.
	assert recurrence_fmt1 == recurrence_fmt3 == recurrence_fmt4

Improve TimeRecurrence class #183

Improve TimeRecurrence class #183

Conversation

MetRonnie commented Sep 10, 2020 • edited Loading

oliver-sanders left a comment • edited by MetRonnie Loading

Choose a reason for hiding this comment

MetRonnie commented Sep 17, 2020

wxtim commented Sep 18, 2020 • edited Loading

wxtim left a comment

Choose a reason for hiding this comment

Essential

Non-essential

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

MetRonnie commented Sep 10, 2020 •

edited

Loading

oliver-sanders left a comment •

edited by MetRonnie

Loading

wxtim commented Sep 18, 2020 •

edited

Loading

oliver-sanders left a comment •

edited

Loading