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

Include sections in UnprocessedTrips #1141

Merged
merged 4 commits into from
Apr 15, 2024

Conversation

JGreenlee
Copy link
Collaborator

To be consistent with processed trips, and so we can use them in the same way, unprocessed trips should have the 'sections' property.

Unprocessed trips / 'draft' trips are assumed to be unimodal, so we can fairly easily reconstruct a section spanning the entirety of the trip and include that as the only section.

In doing this, I did a bunch of refactoring and expanded the type definitions we have for unprocessed trips, composite trips, and adjacent types.

To be consistent with processed trips, and so we can use them in the same way, unprocessed trips should have the 'sections' property.

Unprocessed trips / 'draft' trips are assumed to be unimodal, so we can fairly easily reconstruct a section spanning the entirety of the trip and include that as the only section.

In doing so, I modified points2TripProps. I realized that many of the props will actually be the same between the trip and the section. So I did a bit of refactoring; to construct the unprocessed trip I first construct baseProps (which are used in both the section and the trip); then I construct the section; then finally I construct the trip in the return statement.

Something else I noticed is that all the trip props are computed in points2TripProps except the start_loc and end_loc which were computed in the outer function transitionTrip2TripObj. Since we already have variables for the start and end coordinates in the points2TripProps function, it seems more logical to handle start_loc and end_loc there with the rest of the trip props.

Then we can declare the return type of points2TripProps as UnprocessedTrip; it has all the required properties now.
The main thing I was doing in this commit was adding 'sections' to the type signature of `UnprocessedTrip`. But I also noticed some other things amiss.

`UnprocessedTrip` was missing some other properties; end_ts and start_fmt_time

"LocationCoord" is not needed as it's the same as `Point` from 'geojson'.

`SectionData` was missing a bunch of properties. Once those are filled in, the 'sections' property in `CompositeTrip` and `UnprocessedTrip` can be typed as `SectionData[]`
After a bunch of refactoring, I think these functions could use a naming update.
I always found this part of the codebase fairly opaque anyway and I think it can now be made more easily comprehensible.

'points2TripProps' now returns a full UnprocessedTrip object so it is renamed 'points2UnprocessedTrip'

And 'transitions2Trips' is renamed 'transitions2TripTransitions' because it doesn't really return trip objects; it returns pairs of transitions that represent trips.

To follow suit, 'transitionTrip2TripObj' is renamed 'tripTransitions2UnprocessedTrip'.

Added a bit of JSDoc to help clarify what these functions do.
Filled in more trip object types, and specifically some literal types that define unprocessed trips. (like "source" will always be 'unprocessed').

CompTripLocations changed from `number[]` for coordinates to geojson's `Position`, since that is more descriptive. Renamed CompTripLocations to CompositeTripLocation (this type represents only 1 location).

Used the CompositeTripLocation type in timelineHelper.
in locations2GeojsonTrajectory, the return type needed `properties` for it to be considered a Geojson `Feature`.

formattedSectionProperties types as the return type of the function
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.50%. Comparing base (76839de) to head (826190a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1141      +/-   ##
==========================================
+ Coverage   77.49%   77.50%   +0.01%     
==========================================
  Files          29       29              
  Lines        1693     1694       +1     
  Branches      370      370              
==========================================
+ Hits         1312     1313       +1     
  Misses        381      381              
Flag Coverage Δ
unit 77.50% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
www/js/diary/timelineHelper.ts 94.69% <100.00%> (+0.02%) ⬆️
www/js/types/diaryTypes.ts 50.00% <ø> (ø)

Copy link
Contributor

@jiji14 jiji14 left a comment

Choose a reason for hiding this comment

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

Can you share any related issues or PRs? It would be really helpful for me to fully understand this PR. Thanks!

Comment on lines +71 to +74
origin_key: 'UNPROCESSED_trip';
sections: SectionData[];
source: 'unprocessed';
start_fmt_time: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious! Is there any specific reason why key and origin_key are a combination of uppercase and lowercase letters? It seems unfamiliar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is just how unprocessed trips have always been represented.

This isn't a "real" key because it doesn't come from the server. It's a client-side construction of something similar to a composite_trip object, but it wouldn't really be accurate to call it a de facto composite_trip. So it doesn't get the analysis/composite_trip key.

Somewhere else in the codebase, we identify draft trips by seeing if the key includes 'UNPROCESSED'.

@jiji14
Copy link
Contributor

jiji14 commented Apr 8, 2024

Can you share any related issues or PRs? It would be really helpful for me to fully understand this PR. Thanks!

Oh I found this one! e-mission/e-mission-docs#234 (comment)

@JGreenlee
Copy link
Collaborator Author

JGreenlee commented Apr 10, 2024

Can you share any related issues or PRs? It would be really helpful for me to fully understand this PR. Thanks!

I don't have a lot of written resources on hand, but if you have questions about how unprocessed trips work I can answer them in our 1:1 meeting (and summarize the answers here)

@shankari shankari merged commit 8de3ca0 into e-mission:master Apr 15, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants