Skip to content

Don't indicate stop has been updated when NO_DATA is defined - #3962

Merged
optionsome merged 5 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:no-data-upd
May 20, 2022
Merged

optionsome merged 5 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:no-data-upd

Conversation

@optionsome

@optionsome optionsome commented Mar 4, 2022

Copy link
Copy Markdown
Member

Summary

The realtime indication in the APIs is changed to be false/SCHEDULED for stops that have NO_DATA update from GTFS RT instead of true/UPDATED. This makes it possible (together with #3960) to not have stops marked as having realtime updates if SKIPPED updates are given for other stops in advance before there are time estimates available.

Issue

closes #3961

Unit tests

Added

Code style

Have you followed the suggested code style?
Yes

Documentation

Not needed

Changelog

From title

@optionsome
optionsome requested a review from a team as a code owner March 4, 2022 19:15
@optionsome
optionsome marked this pull request as draft March 7, 2022 18:16
@t2gran t2gran added this to the 2.2 milestone Mar 11, 2022
@optionsome
optionsome marked this pull request as ready for review March 11, 2022 10:13
@t2gran t2gran added !Improvement A functional improvement or micro feature +Real-Time The issue/PR is related to RealTime updates labels Mar 11, 2022
@optionsome

Copy link
Copy Markdown
Member Author

Tests should be added but the tests for GTFS RT updates are currently ignored, see comment #3960 (comment)

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this in the meeting and came to this conclusion

  • use a single a array for the realtime state
  • use top level state

@optionsome

Copy link
Copy Markdown
Member Author

I've updated this pr now so it replaces the boolean flag arrays with an enum state array. I'm not a SIRI expert, so I don't know for sure if it's possible to set a stop as recorded and cancelled at the same time, for example. If it is, is that some information we should keep or should we just store that the stop is cancelled? I think with the current implementation the cancelled state could be overridden by the SIRI updater with recorded/prediction in accurate states which is probably at least a problem if the data allows it.

1 similar comment
@optionsome

Copy link
Copy Markdown
Member Author

I've updated this pr now so it replaces the boolean flag arrays with an enum state array. I'm not a SIRI expert, so I don't know for sure if it's possible to set a stop as recorded and cancelled at the same time, for example. If it is, is that some information we should keep or should we just store that the stop is cancelled? I think with the current implementation the cancelled state could be overridden by the SIRI updater with recorded/prediction in accurate states which is probably at least a problem if the data allows it.

@optionsome optionsome changed the title Don't indicate stop has been updated when NO_DATA is defined when no time updates are given Don't indicate stop has been updated when NO_DATA is defined Mar 16, 2022
@hannesj
hannesj requested a review from lassetyr March 17, 2022 14:46
lassetyr
lassetyr previously approved these changes Mar 21, 2022
hannesj
hannesj previously approved these changes Mar 22, 2022
@MikaelJarfors
MikaelJarfors self-requested a review March 22, 2022 10:24
@optionsome
optionsome dismissed stale reviews from hannesj, lassetyr, and leonardehrenfried via b2ac332 March 24, 2022 09:34
@optionsome

Copy link
Copy Markdown
Member Author

As discussed in the previous developer meeting, I updated the SIRI updater code so that it's not possible that cancellation information is overridden by recorded/inaccurate prediction @lassetyr and @MikaelJarfors .

I still need to update the serialization version id because of the changes to TripTimes right?

hannesj
hannesj previously approved these changes Mar 31, 2022
@MikaelJarfors

Copy link
Copy Markdown
Contributor

Me and Joel had a discussion surrounding this and agreed that it would be good to implement a small unit test that protects the order of the assignments made.

@optionsome

Copy link
Copy Markdown
Member Author

I looked into creating tests yesterday but it didn't seem like the SIRI trip updaters had any. It might take some effort to create tests. Should I still try it or do something else to document this use case?

@optionsome
optionsome dismissed stale reviews from hannesj and leonardehrenfried via 1ce9d97 May 17, 2022 16:49
@optionsome
optionsome marked this pull request as ready for review May 17, 2022 16:51
@optionsome

Copy link
Copy Markdown
Member Author

We discussed this today and came to the conclusion that the tests will be added in some other pull request. As there had been some changes to dev-2.x since the last update on this branch, I rewrote the implementation but the only minor differences to the previous implementation are in the SIRI code.

@optionsome

Copy link
Copy Markdown
Member Author

I remembered that I can now add more tests for the GTFS RT as some of the old tests were fixed. I'll do that.

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GTFS(-RT) parts of this are good.

@t2gran
t2gran requested a review from hannesj May 19, 2022 13:49
hannesj
hannesj previously approved these changes May 20, 2022
Comment thread pom.xml Outdated
@optionsome
optionsome dismissed stale reviews from hannesj and leonardehrenfried via 54dcffc May 20, 2022 08:07
optionsome and others added 2 commits May 20, 2022 11:07
@optionsome
optionsome merged commit 264c702 into opentripplanner:dev-2.x May 20, 2022
@optionsome
optionsome deleted the no-data-upd branch May 20, 2022 10:18
t2gran pushed a commit that referenced this pull request May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature +Real-Time The issue/PR is related to RealTime updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't indicate that a stop has been updated if no time updates are given for any stop

6 participants