⚡️ Use previously-read transition entries instead of re-reading them #1035
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While testing the improvements in e-mission#1017 I noticed that there were multiple calls to `get_data_df` during TRIP_SEGMENTATION, even though we should be reading all the values upfront. e-mission#1017 (comment) e-mission#1017 (comment) On further investigation, this turned out to be from the `is_tracking_restarted_in_range` function, which is used to determine whether we need to add untracked time entries or not. e-mission#1017 (comment) This is O(p), so it is not very intensive, but it is also redundant since we start off by reading all transitions in the active time range into memory. We tried to use the newly added `tracking_restarted_in_loc_df` but it only has comparisons with the previous point. We tried to see if we could determine the range to consider and then check for `all()` but the `tracking_restarted` flag is not available outside of the dist/time subclasses. e-mission#1017 (comment) e-mission#1017 (comment) So we plumbed the transition_df through the codebase and used it directly. `is_tracking_restarted` used to take a timeseries or a transition_df, we have replaced it with the transition_df since we don't want to read from the database any more. Testing done: - all "real data" unit tests pass locally ``` $ ./e-mission-py.bash emission/tests/analysisTests/intakeTests/TestPipelineRealData.py ---------------------------------------------------------------------- Ran 36 tests in 159.777s OK ```
|
Running this on the ccebikes data for our three canonical users with USE_HINTS not set (aka false) we get BeforeAfterThe execution time has gone down from hours to minutes. |
|
@JGreenlee @TeachMeTW for visibility. It may be possible to clean up this code further - e.g. put the dataframes into a thread context instead of passing them around as arguments. But not sure that is the highest priority now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While testing the improvements in
#1017
I noticed that there were multiple calls to
get_data_dfduring TRIP_SEGMENTATION, even though we should be reading all the values upfront.#1017 (comment) #1017 (comment)
On further investigation, this turned out to be from the
is_tracking_restarted_in_rangefunction, which is used to determine whether we need to add untracked time entries or not.#1017 (comment)
This is O(p), so it is not very intensive, but it is also redundant since we start off by reading all transitions in the active time range into memory.
We tried to use the newly added
tracking_restarted_in_loc_dfbut it only has comparisons with the previous point. We tried to see if we could determine the range to consider and then check forall()but thetracking_restartedflag is not available outside of the dist/time subclasses.#1017 (comment) #1017 (comment)
So we plumbed the transition_df through the codebase and used it directly.
is_tracking_restartedused to take a timeseries or a transition_df, we have replaced it with the transition_df since we don't want to read from the database any more.Testing done: