to do list

[ipeagit]

to do list

[mpadge]

@dhersz i'm restructing gtfsrouter to use lots of your great stuff. There is one function which used to live within my extract_gtfs() function which has now been replaced by gtfsio::import_gtfs(), and which is still needed, but would maybe be better living here. It is pure C++, and does one of the items listed above:

• implement set_times_to_seconds().

It works just like these two lines - a single line call that is way faster than any R functions to do this. Let me know and i'll happily PR here.

[dhersz]

Hi @mpadge. This is so good, many thanks!

Just some backstory behind set_times_to_seconds(): I currently use a string_to_seconds() functions I wrote based on tidytransit::hms_to_seconds() (or something along these lines) to convert the time inside functions that require such conversion, such as get_trip_duration(). Your function is certainly much faster than the one I use right now, so I'll gladly start using it instead.

Now, I wanted to have set_times_to_seconds() to save some time when repeatedly calculating the duration of trips, or something like this, because right now get_trip_duration() will always create two new columns arrival_time_secs and departure_time_secs via string_to_seconds() when making such calculations. Therefore, the purpose of set_times_to_seconds() would be to create such columns beforehand, so the user would only need to make this conversion once, instead of every single time they wanted to calculate durations or speeds (hence requiring a check inside get_trip_duration() to see if these columns already exist).

Because your function is so much faster than string_to_seconds(), which was written purely in R, perhaps set_times_to_seconds() as I first thought of it is not as important right now. So to answer your question, I'd keep rcpp_time_to_seconds() as a internal function right now and add a wrapper (set_times_to_seconds()) later.

[dhersz]

note to self: write tests as you write functions

[rafapereirabr]

I think this would be a great contribution, @mpadge .

[mpadge]

Great! It's currently just an internal function that calls directly to the C++ code, so we'll just need to work out whether you might actually want it as some kind of exported and R-wrapped function? I can get a PR together in the meantime regardless, initially with just the internal C++ code, and we can work out where the two lines of R code go:

stop_times [, arrival_time := rcpp_time_to_seconds (arrival_time)]
stop_times [, departure_time := rcpp_time_to_seconds (departure_time)]

[dhersz]

Closing this in favor of individual issues.