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

🎆 Only do BLE matching on the phone for unprocessed BLE Scans #1154

Merged
merged 4 commits into from
May 11, 2024

Conversation

JGreenlee
Copy link
Collaborator

fill in new BLE fields & update types

ble_sensed_mode and ble_sensed_summary were added to sections and confirmed trips in e-mission/e-mission-server#965.

We are going to need ble_sensed_mode to determine vehicle info.

The section summaries (ble_sensed_summary, cleaned_section_summary and inferred_section_summary) are not used in unprocessed trips currently, but I am adding them so there is less of a gap between composite trips and unprocessed trips


use only unprocessed BLE scans in label tab

Since we have matching on the server, processed sections will already have BLE sensed modes filled in. We no longer need to query for all BLE scans; only unprocessed ones (ie newer than pipeline end ts).
Then while we are constructing unprocessed trips, the list of unprocessed BLE scans will be used to determine BLE sensed modes.

We no longer need timelineBleMap and won't treat BLE scans like user inputs anymore. For 'confirmedMode', instead of using timelineBleMap, we will use the ble_sensed_mode of the primary section. New simple function in diaryHelper to deternmine the primary section.

JGreenlee added 3 commits May 7, 2024 13:40
`ble_sensed_mode` and `ble_sensed_summary` were added to sections and confirmed trips in e-mission/e-mission-server#965.

We are going to need `ble_sensed_mode` to determine vehicle info.

The section summaries (`ble_sensed_summary`, `cleaned_section_summary` and `inferred_section_summary`) are not used in unprocessed trips currently, but I am adding them so there is less of a gap between composite trips and unprocessed trips
Since we have matching on the server, processed sections will already have BLE sensed modes filled in. We no longer need to query for all BLE scans; only unprocessed ones (ie newer than pipeline end ts).
Then while we are constructing unprocessed trips, the list of unprocessed BLE scans will be used to determine BLE sensed modes.

We no longer need timelineBleMap and won't treat BLE scans like user inputs anymore. For 'confirmedMode', instead of using timelineBleMap, we will use the ble_sensed_mode of the primary section. New simple function in diaryHelper to deternmine the primary section.
I was just using the latest / master branch to test, but it should really be locked to a release
@JGreenlee JGreenlee marked this pull request as ready for review May 8, 2024 20:47
@JGreenlee
Copy link
Collaborator Author

Unit tests passed but codecov failed

@shankari
Copy link
Contributor

shankari commented May 8, 2024

We should just wait for ~ 30 mins and retry; I will do that
Is there a related server update to bump up common as well?

Copy link

codecov bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 26.47%. Comparing base (f504f68) to head (2b0f4af).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
+ Coverage   26.40%   26.47%   +0.06%     
==========================================
  Files         114      114              
  Lines        4983     4986       +3     
  Branches     1064     1071       +7     
==========================================
+ Hits         1316     1320       +4     
+ Misses       3665     3664       -1     
  Partials        2        2              
Flag Coverage Δ
unit 26.47% <57.14%> (+0.06%) ⬆️

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

Files Coverage Δ
www/js/diary/timelineHelper.ts 92.01% <100.00%> (+0.13%) ⬆️
www/js/diary/LabelTab.tsx 0.00% <0.00%> (ø)
www/js/diary/diaryHelper.ts 69.23% <0.00%> (-4.24%) ⬇️

@JGreenlee
Copy link
Collaborator Author

e-mission-server is fine to stay on 0.4.3 for now because it doesn't use primary_ble_sensed_mode_for_trip yet https://github.com/JGreenlee/e-mission-common/releases/tag/0.4.4.
In fact, neither does e-mission-phone right now.
I need to revise it anyway because of what we discussed on Tuesday

This can be pushed out to fix the slowness on dfc-fermata, while I work on the other changes in parallel

@shankari
Copy link
Contributor

shankari commented May 9, 2024

The pipeline is reset to include ble_sensed_mode properly through the data model. We will need to reset once more after the vehicle_summary_changes are done

Comment on lines -174 to +163
start_ts: pipelineRange.start_ts,
start_ts: pipelineRange.end_ts,
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self, this is not an error, this is the performance optimization!

Comment on lines -339 to +334
const confirmedModeFor = (tlEntry: TimelineEntry) =>
timelineBleMap?.[tlEntry._id.$oid] || labelFor(tlEntry, 'MODE');
const confirmedModeFor = (tlEntry: CompositeTrip) =>
primarySectionForTrip(tlEntry)?.ble_sensed_mode || labelFor(tlEntry, 'MODE');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, future fix: I think it would be worthwhile for you and Abby to sync up on how this should work across the public dashboard and the phone. The server side data does not have a confirmedMode; maybe it should add one. Also, I am not sure that confirmed_mode is the best name for this, since the ble_sensed_mode has not been confirmed by anybody.

Copy link
Collaborator Author

@JGreenlee JGreenlee May 11, 2024

Choose a reason for hiding this comment

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

Agreed, this is not the long term solution.

"Confirmed mode" was something temporary I threw together for the alpha. When I first added it, it actually represented the BLE sensed mode OR the labeled mode, depending on what was available.

For dfc-fermata there are no mode labels and I think it makes more sense to use something like primary_ble_sensed_mode. will coordinate this with Abby once I know what adjustments we are making to the section summaries

I do think something like "confirmed mode" will be useful in the app dashboard and for analysis. I'm envisioning future programs that could theoretically use BLE sensing in addition to mode+purpsoe labels – the BLE sensed trips will not need manual user input since we already "confirmed" the mode when we matched it to a beacon.
When we do viz/ analysis we will want one property that encompasses both cases: i) determined by BLE or ii) manually confirmed

@shankari shankari merged commit 681d2f1 into e-mission:master May 11, 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.

2 participants