Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ETA's, Formating #25

Merged
merged 13 commits into from
Oct 1, 2024
Merged

ETA's, Formating #25

merged 13 commits into from
Oct 1, 2024

Conversation

dmv1167
Copy link

@dmv1167 dmv1167 commented Sep 29, 2024

Take a look at what I've worked on and let me know what you think. I did implement that other API for the ETA's because I couldn't figure out where he was getting them from. I need to monitor the busses on PassioGo and check them against these estimates sometime. Docstrings for what I added will need to be added in the future.

Added optional return types, removed None comparison with == and some redundant parentheses.
Began testing on live timing.
Added getStopById to Routes and Systems for use with live timing.
Vehicle getNextStop() and getEtas() use 3rd party API to get stops and times.
Stop getNextVehicle() and getEtas() gets vehicles and times.
This was referenced Sep 29, 2024
@athuler
Copy link
Owner

athuler commented Sep 29, 2024

Thanks for the Pull Request! Could you split Caching (#14 ) as a separate PR? It's a feature that requires further testing and isn't quite ready to be released with 0.3.0.

The formatting and ETA stuff look great at a glance!

@dmv1167 dmv1167 changed the title ETA's, Caching, Formatting ETA's, Formating Sep 29, 2024
@dmv1167
Copy link
Author

dmv1167 commented Sep 29, 2024

Alright I have removed caching to another pull, I had originally implemented that because my eta methods were taking forever due to repeat calls to get methods.

Eta's now come straight from Passio, I don't know if the (Stop, eta) and (Vehicle, eta) tuples I return from these methods is the most useful option as there are tons of fields that users may want access to. Maybe an ETA class? I don't want to stray too far from your view of the project.

I feel like there's a better solution for how I generate the unix timestamps as there are tons of edge cases to consider. The etaR field of each bus seems to hold a better formatted version of the eta, perhaps that could be used in some way.

@athuler
Copy link
Owner

athuler commented Sep 29, 2024

It looks like etaR is the minutes until the bus arrives and secondsSpent gives the actual estimation in seconds. I think the method should only return the latter.

With regards to the Stop object, a single stop is frequently part of multiple routes, which PassioGo doesn't specify when fetching each stop. So we cannot store a single routeId or route object attribute.

If we want to add a list attribute of Route objects to each Stop, we'd have to iterate through each Route object within a system and add each Route object with the stop id. If you think that's something that could be useful, you're free to give it a shot! (in a new PR)

@athuler athuler changed the base branch from main to 0.3.0 September 29, 2024 17:09
@dmv1167
Copy link
Author

dmv1167 commented Sep 29, 2024

You're right, I attend a university that uses this system and still forgot that a stop can have multiple routes. So we should really be looking at secondsSpent, and just return that. I do hope to figure out how to specify what Vehicle will be arriving to a Stop next and conversely what Stop a Vehicle will be at next, at least that was what I was hoping to get for the project I'm using this in.

With the use of secondsSpent I'll be able to forego the library that I believe is causing these checks to fail.

I also apologize if I haven't followed contribution conventions, I'm new to the process.

@athuler
Copy link
Owner

athuler commented Sep 29, 2024

No worries, I appreciate the help!

The Stop.getEtas() method could return a list of tuples such as [(secondsUntilArrival, <Vehicle>), (...,...),...] which would (through the Vehicle object) give us route information.

What was the intended purpose of your Vehicle.getEtas() method?

I don't think getting a vehicle's next stop is natively supported by PassioGo's API, but it doesn't seem to be impossible. Feel free to open an issue for it!

I would greatly appreciate it if you could add docstrings to as many of the new functions you're writing to make collaboration easier. Thanks!

@dmv1167
Copy link
Author

dmv1167 commented Sep 29, 2024

Vehicle.getEtas() was intended to allow you to see what Stop a specific bus would be at next, but I suppose that information is less useful. I implemented it by iterating through the API calls for every Stop eta in the System, then pulling only those that matched to the Vehicle.id. I can remove that for now.

To confirm, your idea for Stop.getEtas() replaces the need for the Stop to store the Route objects it's a part of?
Also, Stop.getEtas() currently returns a list of tuples like [(unixArrivalTimestamp, <Vehicle>), (...,...),...], would you like me to represent it as secondsUntilArrival instead?

@athuler
Copy link
Owner

athuler commented Sep 29, 2024

As of right now, it's not necessary to have the Stop object store a list of Route objects, but that's something you can work on for a future feature if you would like!

We could make Stop.getEtas() take a boolean argument returnInUTC=False which determines whether it returns the number of seconds remaining or the estimated time of arrival in UTC.

@dmv1167
Copy link
Author

dmv1167 commented Sep 29, 2024

Yeah I didn't think so either, I just wanted to make sure I understood. I'll add the Boolean argument soon.

@athuler
Copy link
Owner

athuler commented Oct 1, 2024

Given that TransportationSystem.getStopById() exists, is it necessary for Route.getStopById() to also be present? I argue it is an unnecessary duplicate and should be removed.

If the intent is to check whether a stop exists, a user could simply check Route.getStops().

Let me know what your thinking behind this was!

@dmv1167
Copy link
Author

dmv1167 commented Oct 1, 2024

Route.getStopById() is purely for performance reasons as it would only have to check its own Stops. However, Route.getStops() currently has to check every Stop in the System anyways, so the performance gains can only be realized if used in conjunction with the caching I had previously come up with.

To be specific, in my testing with Stops stored with each Route object, the first run of Route.getStops() takes ~0.5 seconds, and every subsequent run takes ~4e10-6 seconds. If my math is correct, this is 125,000 times faster.

If we'd like to remove the duplicate until it can be more useful that works, or if you'd rather scrap it permanently we can.

This is also the case with Route.getVehicleById().

@athuler
Copy link
Owner

athuler commented Oct 1, 2024

I think that's a very good idea but let's move it to the caching branch as that is its primary purpose.

@athuler athuler merged commit aafeb65 into athuler:0.3.0 Oct 1, 2024
4 checks passed
@athuler athuler mentioned this pull request Oct 1, 2024
@athuler athuler linked an issue Oct 1, 2024 that may be closed by this pull request
@dmv1167 dmv1167 mentioned this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ETAs
2 participants