[feature] keep forbidden pickup/dropoff on stay in #730

antoine-de · 2021-01-11T16:14:14Z

To keep to data as clean as possible, the model was sanitized to add restrictions on drop off at first stop time / pick up at last stop time.
Since for stay in this could remove valid journeys, the pick up at last stop time / drop off at first stop time was possible for stay in.

The previous implementation was removing explicitly set constraints on those stay in stop times.
As a side effect we now also correctly handle restrictions on stay in of different calendars (see example 4)

Note 1: the new implementation is quadratic in term of trip on the same block_id. However the number of vj on the same block_id seems quite limited thus the runtime penalty is quite small.
I tested it on the bretagne dataset, so biggest dataset I know with block_id (106k trips, 2M+ stoptimes, max number of vehicle journeys with same block_id: 76) there is a small impact on the run time import (not on the memory footprint).
On my computer the best of 3 runs (there are ~1s of difference on the same runs) before: 17s to read the GTFS, after: 18s to read the GTFS.

Note 2:
this reactivate the use of the model_builder and add some utilities to it

internally linked to ND-1088

(tdd power !)

refactor enhance_pickup_dropoff to keep restrictions set

and add some calendars helpers

antoine-de · 2021-01-11T18:36:25Z

I should definitly have split this PR in 2, one for the builder and one for the bug.

I can still do it if you prefer.

I'd be happy to have a review on the builder's API

pbougue

Did not review the model-builder part (vote for a separate PR, as we are not in a hurry to merge any of this work).

👍 Despite remarks on twisted cases, it's better than what we had on staying as it respects data more.

pbougue · 2021-01-12T18:08:22Z

src/model.rs

+                        // The stay in is not really possible when timing overlaps
+                        // between arrival of first vehicle journey and departure of
+                        // next vehicle journey (see Example 2 above).
+                        last_stop.departure_time <= first_stop.arrival_time


If one is past-midnight and the next is at the beginning of the day after, I guess it won't work but it "only" matters for night-bus lines
🙈 becomes a bit too complex for the gain I think.

after giving it some thoughts, past midnight are usually implemented as time > 24h (like "25:00:00" for 1am) no? and this case is handled no 🤔 ?

It would not be handled I think, as the first VJ would end at 25:00 and the second would start at 02:00.
They would be possible stay-in, but we wouldn't "see" that.
But that's OK for now (and possibly for ever 😝 )
I do not resolve the comment just to have it quick if we ever come back later to that work to improve those tricky cases.

src/model.rs

pbougue · 2021-01-12T18:34:21Z

src/model.rs

+                            if can_chain_without_overlap(vj, other_vj) {
+                                allowed_first_drop_off_vj.insert(*other_vj_idx);
+                                allowed_last_pick_up_vj.insert(vj_idx);
+                            } else if can_chain_without_overlap(other_vj, vj) {
+                                allowed_first_drop_off_vj.insert(vj_idx);
+                                allowed_last_pick_up_vj.insert(*other_vj_idx);


Not sure how twisted this example is (all vjs are on the same block_id, same calendar):
vj1 ends at spA (10:00-11:00)
vj2 starts at spA (10:00-11:00) ends at spB (12:00-13:00)
vj3 starts at spC (14:00-15:00)

Because of vj1-vj3 test, the outcome from your code is that all vj1's last dropoff is allowed at spA which is not what we want, no?
(we would have the same for vj2's first pickup if we had a vj0).

This probably implies building a chain first (tree if you consider calendars), then check what's possible... then decide what matters more in case of conflict (probably being safe and forbid).
And it's actually what's done in Navitia as the next vj is unique and check is done once it's affected.

hum interesting edge case, I don't know if it's a valid GTFS 🤔
I'm not sure it's worth implementing something this tricky as this is meant only for strengthening the data if we do in transit_model I don't think we should be "safe and forbid" (like we should in navitia) since we are only adding additional constraints on the data. So we should just forbid impossible cases, and if a particular case can happen sometimes but not always, we should let it be possible and be handled by the journey planner

I think this should be split into 2 parts :

Is it a realistic case to have chained blocks (so every extremity would endup being qualified as a valid stay-in in this algo) ? I think we get block_id corresponding to vehicles, so yes (so an algo without determining "real-life" chains would not change data much from original).

Is it realistic to have calendars that would make "real-life" chaining of trips being stay-in some days and not stay-in some other days? I think we should not care about that until we have the problem as it's too edgy 🙈 . And OK to stay closer to data in that conflicting case, you make a good point contradicting my "safe" argument.

As discussed:

Not addressing that for now, so authorize a bit too much pickup/dropoff which is OK (still better to respect data). To be documented but that's all.

If calendars introduce a contradictory case (stay-in should be authorized on sunday, not on saturday) : authorise to respect data. To be tested.

test added block_id_on_non_overlaping_calendar_with_overlaping_stops (and I think the current implementation handles it quite nicely after testing it 😜 )

Indeed, it's nice : what's consistent is handle, and what's not stays close to initial data 👍
Letting the discussion unresolved for the "chaining" part if we later come back to that work, but you did all that could reasonably be done ✔️

src/model.rs

woshilapin

Overall, it's an improvement since we keep existing values and there is more test. But I feel like there is still a bunch of work to do to make it maintainable.

src/model.rs

Co-authored-by: Jean SIMARD <[email protected]>

ArnaudOggy · 2021-01-15T14:09:43Z

To bump in 0.34 and its sub-crates 😉

src/model.rs

pbougue

ModelBuilder interface with default attributes + muter looks fine, and to me we settled on something good for functionnal stay-in part 👍
Not sure about the fallible IntoDate, and didn't look at all @woshilapin's unresolved comments, but that's the only review left before approval.

model-builder/src/builder.rs

src/model.rs

antoine-de · 2021-01-18T10:33:46Z

I think I resolved all issues, feel free to poke me again if I missed something (apart from the cfg(test) but I'm not convinced it is needed and I did not manage to add it (I did not dig much I confess though 😜 ))

pbougue

Looks like all is addressed indeed (let @woshilapin check his last opened comments, though)

woshilapin

Still need to bump Cargo.toml to 0.33.1 at least (or 0.34.0 as @ArnaudOggy suggested, but I would say this is bug fix so 0.33.1 is probably enough). The rest of the PR is good-to-go for me.

ArnaudOggy · 2021-01-18T13:00:41Z

Still need to bump Cargo.toml to 0.33.1 at least (or 0.34.0 as @ArnaudOggy suggested, but I would say this is bug fix so 0.33.1 is probably enough). The rest of the PR is good-to-go for me.

No need 0.34.0 with the re-entry of model-builder? I let you decide what is best 😉

antoine-de added 6 commits January 8, 2021 14:44

plug back the model builder

f05a754

add a failing test

5beea9f

(tdd power !)

refactor enhance_pickup_dropoff

27fa25d

refactor enhance_pickup_dropoff to keep restrictions set

rename st_init to st_mut for coherence

018f83b

and add some calendars helpers

cleanup test and add comment

0fe25b4

check calendars overlap

dc98c19

antoine-de force-pushed the odt_kept branch from e4ccfc7 to fd97d57 Compare January 11, 2021 16:56

add metadata on model-builder package

820eae0

antoine-de force-pushed the odt_kept branch from fd97d57 to 820eae0 Compare January 11, 2021 18:14

clippy

39b9259

pbougue requested review from woshilapin and pbougue January 12, 2021 09:51

pbougue reviewed Jan 12, 2021

View reviewed changes

woshilapin suggested changes Jan 13, 2021

View reviewed changes

antoine-de and others added 8 commits January 13, 2021 08:48

Update src/model.rs

892e907

Co-authored-by: Jean SIMARD <[email protected]>

Update src/model.rs

8c1cbb9

Co-authored-by: Jean SIMARD <[email protected]>

Update src/model.rs

6e917fd

Co-authored-by: Jean SIMARD <[email protected]>

Update src/model.rs

c4e0d7a

Co-authored-by: Jean SIMARD <[email protected]>

Update src/model.rs

e56eb8a

Co-authored-by: Jean SIMARD <[email protected]>

Update src/model.rs

44ba1f0

Co-authored-by: Jean SIMARD <[email protected]>

Update src/model.rs

fbb967f

Co-authored-by: Jean SIMARD <[email protected]>

Update src/model.rs

64a7e5c

Co-authored-by: Jean SIMARD <[email protected]>

antoine-de added 2 commits January 15, 2021 15:43

corrections after review

998ba1d

add another test on a tricky case

bb88181

pbougue reviewed Jan 15, 2021

View reviewed changes

src/model.rs Show resolved Hide resolved

pbougue reviewed Jan 15, 2021

View reviewed changes

model-builder/src/builder.rs Show resolved Hide resolved

model-builder/src/builder.rs Show resolved Hide resolved

src/model.rs Outdated Show resolved Hide resolved

antoine-de force-pushed the odt_kept branch 2 times, most recently from a86382c to 156a9a2 Compare January 18, 2021 09:44

antoine-de force-pushed the odt_kept branch from 156a9a2 to 8a594c1 Compare January 18, 2021 10:13

antoine-de added 3 commits January 18, 2021 11:19

use array for calendar dates

0842cc6

add test comment

1e7e1fc

add better panic msg

fa95b1b

antoine-de force-pushed the odt_kept branch from 8a594c1 to fa95b1b Compare January 18, 2021 10:20

add comment on possible fail

eb56cbc

pbougue previously approved these changes Jan 18, 2021

View reviewed changes

woshilapin suggested changes Jan 18, 2021

View reviewed changes

bump version

4ba81b2

antoine-de dismissed pbougue’s stale review via 4ba81b2 January 18, 2021 13:06

pbougue approved these changes Jan 18, 2021

View reviewed changes

woshilapin approved these changes Jan 22, 2021

View reviewed changes

antoine-de merged commit 663c273 into hove-io:master Jan 25, 2021

antoine-de deleted the odt_kept branch January 25, 2021 10:07

pbougue changed the title ~~keep forbidden pickup/dropoff on stay in~~ [feat] keep forbidden pickup/dropoff on stay in Feb 1, 2021

datanel changed the title ~~[feat] keep forbidden pickup/dropoff on stay in~~ [feature] keep forbidden pickup/dropoff on stay in Feb 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] keep forbidden pickup/dropoff on stay in #730

[feature] keep forbidden pickup/dropoff on stay in #730

antoine-de commented Jan 11, 2021

antoine-de commented Jan 11, 2021

pbougue left a comment

pbougue Jan 12, 2021

antoine-de Jan 18, 2021

pbougue Jan 18, 2021

pbougue Jan 12, 2021

antoine-de Jan 12, 2021

pbougue Jan 13, 2021 •

edited

Loading

pbougue Jan 15, 2021

antoine-de Jan 18, 2021

pbougue Jan 18, 2021

woshilapin left a comment

ArnaudOggy commented Jan 15, 2021

pbougue left a comment

antoine-de commented Jan 18, 2021 •

edited

Loading

pbougue left a comment

woshilapin left a comment •

edited

Loading

ArnaudOggy commented Jan 18, 2021 •

edited

Loading

[feature] keep forbidden pickup/dropoff on stay in #730

[feature] keep forbidden pickup/dropoff on stay in #730

Conversation

antoine-de commented Jan 11, 2021

antoine-de commented Jan 11, 2021

pbougue left a 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

pbougue Jan 13, 2021 • 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

woshilapin left a comment

Choose a reason for hiding this comment

ArnaudOggy commented Jan 15, 2021

pbougue left a comment

Choose a reason for hiding this comment

antoine-de commented Jan 18, 2021 • edited Loading

pbougue left a comment

Choose a reason for hiding this comment

woshilapin left a comment • edited Loading

Choose a reason for hiding this comment

ArnaudOggy commented Jan 18, 2021 • edited Loading

pbougue Jan 13, 2021 •

edited

Loading

antoine-de commented Jan 18, 2021 •

edited

Loading

woshilapin left a comment •

edited

Loading

ArnaudOggy commented Jan 18, 2021 •

edited

Loading