Open mmmvvvppp opened 1 year ago
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:
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.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.
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.
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_id
s 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
.
The
teacher_room_stability
method in the School Timetabling example uses a Joiner to compare Lessons of different ids, butteacher_time_efficiency
andstudent_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 thefor_each
calls tofor_each_unique_pair
.