search

tijlleenders / ZinZen-scheduler

The calendar engine for the ZinZen web app.
https://github.com/tijlleenders/ZinZen
GNU Affero General Public License v3.0
10 stars 4 forks source link

rom/411/new scheduler - new Scheduler PoC #423

Closed thinkrapido closed 11 months ago

thinkrapido commented 11 months ago

Ready for Review and merge

tijlleenders commented 11 months ago

@thinkrapido I guess this PR is not ready for review yet right? Can I convert the status to 'draft'? Is there anything @kobe-reygel could add to the PR to help? Maybe removing all the old stuff?

thinkrapido commented 11 months ago

@tijlleenders now it's ready for merging. @kobe-reygel , please review

thinkrapido commented 11 months ago

@kobe-reygel overall you did a great job highlighting my shortcummings. I'm quite busy this week, but I'll try what I can.

kobe-reygel commented 11 months ago

@kobe-reygel overall you did a great job highlighting my shortcummings. I'm quite busy this week, but I'll try what I can.

Don't worry @thinkrapido , I'm also busy this week (and weekend)😀

If I can help somewhere, let me know!

tijlleenders commented 11 months ago

@thinkrapido

I've also stepped through the test case.
The order of processing the goals and the logic follows the algorithm, nice work!

I have a few questions:

  1. Why are we introducing a new concept min_span?
    How is this different from min_duration - which aligns better with the frontend/domain language?

  2. Once a goal is being handled - and it's not impossible of flex 1 - the logic of finding the right slot is quite obscure to me.
    What is the tail we're introducing here? image

  3. Can we find a better name for the Day struct? This word/concept is not in the algorithm.
    Day is part of the Flexibility concept (also new but I think useful) which also holds a ref to a goal. So Flexibility keeps track of the possible places on the Calendar for a specific goal? I think a more appropriate name for Day then is Timeline - as it could itself could contain multiple Slots and doesn't necessarily have to align with calendar days. Currently Day does align with calendar days, and always contains one Slot.

  4. Seats is another new concept - that doesn't really add anything in my opinion. What is it and why can't we remove it?

kobe-reygel commented 11 months ago

@tijlleenders @thinkrapido

  1. Why are we introducing a new concept min_span? How is this different from min_duration - which aligns better with the frontend/domain language?

=> as I understand it - it is the same. Possibly a new name to stop confusion with the old implementation. I agree that we need to align this with the domain language

  1. Once a goal is being handled - and it's not impossible of flex 1 - the logic of finding the right slot is quite obscure to me. What is the tail we're introducing here? image

I had the same objection about obscurity. The logic is sound however. The entire tail/ selected block is basically as follows: 'select the element that we want to take (the position we want), partition the unprocessed task list on this elemt so that we select the element. The tail contains all remaining elements.' => basically the functional programming equivalent of selectedElement = list.pop(). The logic is correct as far as I can see. Let's make a 'readability improvement ticket' for this, but not block the merging on this.

  1. Can we find a better name for the Day struct? This word/concept is not in the algorithm. Day is part of the Flexibility concept (also new but I think useful) which also holds a ref to a goal. So Flexibility keeps track of the possible places on the Calendar for a specific goal? I think a more appropriate name for Day then is Timeline - as it could itself could contain multiple Slots and doesn't necessarily have to align with calendar days. Currently Day does align with calendar days, and always contains one Slot.

agree

  1. Seats is another new concept - that doesn't really add anything in my opinion. What is it and why can't we remove it?

I understand it as being the same as the old concept of 'Slots' in a timeline. Agree that we should align this with existing terminology