Closed tpaviot closed 3 years ago
Merging #87 (0aca000) into master (c821577) will increase coverage by
0.22%
. The diff coverage is98.66%
.
@@ Coverage Diff @@
## master #87 +/- ##
==========================================
+ Coverage 94.96% 95.19% +0.22%
==========================================
Files 30 31 +1
Lines 3419 3643 +224
==========================================
+ Hits 3247 3468 +221
- Misses 172 175 +3
Impacted Files | Coverage Δ | |
---|---|---|
processscheduler/__init__.py | 87.50% <ø> (ø) |
|
processscheduler/resource_constraint.py | 96.11% <94.73%> (-0.86%) |
:arrow_down: |
test/test_resource_tasks_distance.py | 99.46% <99.46%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update c821577...0aca000. Read the comment docs.
Please test, there seems to be an issue with the task duration (space is 6 whereas it should be 8).
Hey Thomas, were you able to fix this issue?
def test_resource_tasks_distance_5(self) -> None:
"""Adding one scheduled optional tasks"""
pb = ps.SchedulingProblem("ResourceTasksDistance5OptionalTasks", horizon=30)
task_1 = ps.FixedDurationTask("task1", duration=8)
task_2 = ps.FixedDurationTask("task2", duration=4)
task_3 = ps.FixedDurationTask("task3", duration=3, optional=True)
worker_1 = ps.Worker("Worker1")
task_1.add_required_resource(worker_1)
task_2.add_required_resource(worker_1)
task_3.add_required_resource(worker_1)
c1 = ps.ResourceTasksDistance(worker_1, distance=4, mode="exact")
pb.add_constraint(c1)
pb.add_constraint(ps.TaskStartAt(task_1, 1))
solver = ps.SchedulingSolver(pb)
# for optional tasks to not be scheduled
solver.add_constraint(task_3.scheduled == True)
solution = solver.solve()
self.assertTrue(solution)
t1_start = solution.tasks[task_1.name].start
t2_start = solution.tasks[task_2.name].start
t3_start = solution.tasks[task_3.name].start
t1_end = solution.tasks[task_1.name].end
t2_end = solution.tasks[task_2.name].end
t3_end = solution.tasks[task_3.name].end
self.assertEqual(t1_start, 1)
self.assertEqual(t1_end, 9)
self.assertTrue(t2_start == 13 or t3_start == 13)
if t2_start == 13:
self.assertEqual(t2_end, 17)
self.assertEqual(t3_start, 21, f'{t3_start=} {t3_end=}')
self.assertEqual(t3_end, 24)
elif t3_start == 13:
self.assertEqual(t3_end, 16)
self.assertEqual(t2_start, 20)
self.assertEqual(t2_end, 24)
Testing with scheduled optional task and working as expected!!
The next step for this constraint was to make it admit periods, so the distance is applied inside certain periods and not outside of them. For example, resource A tasks distance should be 4 in periods [(0,20), (23, 40)]. We have to decide if each constraint should admit a single period, and in order to do the example, 2 constraints would need to be added, one for (0,20) and another one for (23, 40); or if each constraint can deal with multiple periods like in the example.
Thanks!
@dreinon Yes, it works with optional tasks as well. That should not be very hard to add a time restriction to this constraint, although I don't really understand the use case. The documentation should explicit this clearly, how to use the constraint and what kind of problem this constraint represents.
Right, I can doc it myself.
My use case is that in my problem, I have several periods that are not consecutive one from each other, for example, from 9h to 13:30h, then from 14h to 16:15h and then from 17:15h to 21h, all with 45 min task duration. In real life, these three periods are clearly separated by time, although in the ProcessScheduler problem representation, since we are dealing with integers, 9h to 13:30h would be (0,6), then 14h to 16:15h would be (6,9) and then 17:15h to 21h would be (9,14).
This is modelled this way for 3 reasons:
Therefore, if the constraints didn't have periods, then distance wouldn't be real. Imagine now the end of a day and the start of another day of the week. They share the same integer timepoint even though they are really apart in real time. Imagine an equal distance of 0 between tasks of the same resource constraint is used without specifying periods. Then, a possible case would be that one task ended a day and another one started a day, and that would have distance 0 in the problem, which wouldn't work for real life because those two periods are actually not even close.
I hope I could explain my use case well and that you get what I was trying to explain :)
ok, I will add a time_periods
parameter, for example time_periods=[(0,6), (6,9), (9,14)]
in which the distance constraint applies
Thanks!
❌ Merging this PR will decrease code quality in the affected files by 1.67%.
Quality metrics | Before | After | Change |
---|---|---|---|
Complexity | 9.77 🙂 | 10.21 🙂 | 0.44 👎 |
Method Length | 77.67 🙂 | 96.12 🙂 | 18.45 👎 |
Working memory | 11.02 😞 | 10.94 😞 | -0.08 👍 |
Quality | 53.25% 🙂 | 51.58% 🙂 | -1.67% 👎 |
Other metrics | Before | After | Change |
---|---|---|---|
Lines | 212 | 281 | 69 |
Changed files | Quality Before | Quality After | Quality Change |
---|---|---|---|
processscheduler/init.py | 83.93% ⭐ | 83.77% ⭐ | -0.16% 👎 |
processscheduler/resource_constraint.py | 51.51% 🙂 | 50.47% 🙂 | -1.04% 👎 |
Here are some functions in these files that still need a tune-up:
File | Function | Complexity | Length | Working Memory | Quality | Recommendation |
---|---|---|---|---|---|---|
processscheduler/resource_constraint.py | WorkLoad.__init__ | 13 🙂 | 257 ⛔ | 14 😞 | 35.94% 😞 | Try splitting into smaller methods. Extract out complex expressions |
processscheduler/resource_constraint.py | ResourceTasksDistance.__init__ | 14 🙂 | 223 ⛔ | 12 😞 | 39.95% 😞 | Try splitting into smaller methods. Extract out complex expressions |
The emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.
Please see our documentation here for details on how these metrics are calculated.
We are actively working on this report - lots more documentation and extra metrics to come!
Help us improve this quality report!
I committed an update with time periods, that should work. You can have a look at the related unittest as well, see https://github.com/tpaviot/ProcessScheduler/pull/87/commits/0aca00075e2c1dec277580d63d0068eb4597f689
Sourcery is good, but a bit too much intrusive
Sourcery is good, but a bit too much intrusive
Right, I like that but I understand it's not for everybody :smile:
Thanks!
Related to #83
Please @dreinon can you test