Comparison against Lesson.id missing from School Timetabling

[mmmvvvppp]

The teacher_room_stability method in the School Timetabling example uses a Joiner to compare Lessons of different ids, but teacher_time_efficiency and student_group_subject_variety do not. I believe this is an error and will introduce a comparison between two copies of the same Lesson. Since all these constrains compare unique instances of Lessons, it might make sense to change all the for_each calls to for_each_unique_pair.

[Christopher-Chianelli]

We have tests to verify the constraints are working correctly: https://github.com/optapy/optapy-quickstarts/blob/4bdf9cce4a5e1ed4ffb3691b940abb0168251d63/school-timetabling/tests.py#L57-L79

In particular:

• We don't use abs in https://github.com/optapy/optapy-quickstarts/blob/4bdf9cce4a5e1ed4ffb3691b940abb0168251d63/school-timetabling/constraints.py#L11-L13, meaning the difference can be negative. This means for_each_unique_pair will not work here, since for different lessons A, B, one of the pairs (A, B) or (B, A) will have a negative difference (and thus, the constraint might not get triggered if the "wrong" pair is selected by for_each_unique_pair). If we did an abs on between, then for_each_unique_pair can be used.
• As for why it does not cause an issue, there is an input constraint: all timeslots are 1 hour long, which is greater than 30 minutes. This means between = datetime.combine(today, lesson.timeslot.end_time) - datetime.combine(today, lesson.timeslot.start_time) will always be 1 hour, which is greater than 30 minutes, thus within_30_minutes return False.

However, this bug will appear if the input constraint is removed and timeslots are less than or equal to 30 minutes. These two constraints are identical to the ones in optaplanner-quickstarts (https://github.com/kiegroup/optaplanner-quickstarts/blob/531a0b277c386b3d518f147f3271c054337f9c46/use-cases/school-timetabling/src/main/java/org/acme/schooltimetabling/solver/TimeTableConstraintProvider.java#L72-L102) which means optaplanner-quickstarts also have this bug.

[Christopher-Chianelli]

Upon further inspecting, optaplanner-quickstarts would not have the bug since it does start - end, which is always negative for the pair (A, A). Since we do end - start, which is always positive for the pair (A, A), the bug would appear in optapy-quickstarts if the input constraints were relaxed.

[mmmvvvppp]

Thanks for thorough reply! I would say you've answered my original issue as to why (A, A) pairings do not cause the constraint to be triggered. Since a pairing (A, B) is guaranteed to have a particular ordering (is A the lower-valued planning entity?), I can induce failure if I swap planning_ids 2 and 3:

def test_teacher_time_efficiency(): 
     teacher = "Teacher1" 
     single_lesson_on_monday = Lesson(1, "Subject1", teacher, "Group1", TIMESLOT1, ROOM1) 
     first_tuesday_lesson = Lesson(3, "Subject2", teacher, "Group2", TIMESLOT2, ROOM1) 
     second_tuesday_lesson = Lesson(2, "Subject3", teacher, "Group3", TIMESLOT3, ROOM1) 
     third_tuesday_lesson_with_gap = Lesson(4, "Subject4", teacher, "Group4", TIMESLOT4, ROOM1) 
     constraint_verifier.verify_that(teacher_time_efficiency) \ 
         .given(single_lesson_on_monday, first_tuesday_lesson, second_tuesday_lesson, third_tuesday_lesson_with_gap) \ 
         .rewards_with(1)  # Second tuesday lesson immediately follows the first. 

My expectation is that the outcome should be invariant with the value of the planning_id used in the Lesson.