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.

Sign up for GitHub

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

Jump to bottom

🚑 labels "lost" and performance issues when labeling with current staging #1034

Closed
shankari opened this issue Nov 26, 2023 · 4 comments
Closed

🚑 labels "lost" and performance issues when labeling with current staging #1034

shankari opened this issue Nov 26, 2023 · 4 comments

Comments

@shankari
Copy link
Contributor

shankari commented Nov 26, 2023 •
edited
Loading

I have noticed two issues while labeling with the new release.

  1. Labeling is consistently slow
  2. Labels are sometimes "lost" and only restored after reload. This only happened once, when I was on cellular data (at the Santa Clara Caltrain station).

I have uploaded videos and logs to an internal issue to avoid having to go through and remove all personal information.
After investigation there, we should add a summary here.
@shankari shankari moved this to Showstoppers for the current release in OpenPATH Tasks Overview Nov 28, 2023
@JGreenlee
Copy link

(1) is getting fixed by e-mission/e-mission-phone#1106. Labeling is now quite snappy on that branch.

@JGreenlee
Copy link

  1. It takes two taps of the "yellow checkmark" to mark a pair of inferred labels as verified
  2. When an addition / timeuse survey response is recorded, the timestamps initially show up as blank

JGreenlee added a commit to JGreenlee/e-mission-phone that referenced this issue Jan 8, 2024
@JGreenlee
update unprocessed inputs after recording user input
961ac78 
-- Part of the fix for e-mission/e-mission-docs#1034

When a user input is recorded, we put it in storage. For snappy performance we also cache it in the timelineLabelMap / timelineNotesMap state.
However, if we switch filters or load more trips, this state gets overridden. So, we need to call updateLocalUnprocessedInputs at some point.
It is an async function but we can call it without 'await' and just allow it to execute in the background.
JGreenlee added a commit to JGreenlee/e-mission-phone that referenced this issue Jan 8, 2024
@JGreenlee
allow labels to be stored as a batch
68ae2bb 
-- Part of the fix for e-mission/e-mission-docs#1034

When the yellow checkmark is used to verify a pair of inferred mode+purpose labels, both labels should be verified by a single click.
The labels get stored fine; however, the way we update state after doing so prevents both labels from being updated simultaneously and only one of them gets filled in. The resulting UX is that it takes 2 clicks and this is not desired.
We could: update state in succession, one after the other (await label A, then proceed label B),
OR we could rework these functions to support adding labels for a particular trip as a batch (so the same function call could store mode+purpose at the same time). This was decided as a better and more performant option.

It uses Promise.all to store all labels from the batch, then it updates the state with all changes from that batch.
When a single label is recorded, it is a batch of 1 - when verifying inferences, it may be a batch of multiple labels.
@JGreenlee
Copy link

JGreenlee commented Jan 8, 2024 •
edited
Loading

(2) and (3) are fixed in e-mission/e-mission-phone#1106 by e-mission/e-mission-phone@961ac78 and e-mission/e-mission-phone@68ae2bb

JGreenlee added a commit to JGreenlee/e-mission-phone that referenced this issue Jan 9, 2024
@JGreenlee
use memoization pattern for added notes' displayDt
1474833 
Instead of mutating an extra property 'displayDt' onto the entries, let's use a get function that returns the displayDt. This should fix issue (4) in e-mission/e-mission-docs#1034.
And let's also memoize that function so performance is not impacted
@JGreenlee
Copy link

JGreenlee commented Jan 9, 2024 •
edited
Loading

All of these are now fixed on e-mission/e-mission-phone#1106.
I will add videos later showing the issues are no longer present so that this can be closed

JGreenlee added a commit to JGreenlee/e-mission-phone that referenced this issue Jan 25, 2024
@JGreenlee
update unprocessed inputs after recording user input
a6e7a79 
-- Part of the fix for e-mission/e-mission-docs#1034

When a user input is recorded, we put it in storage. For snappy performance we also cache it in the timelineLabelMap / timelineNotesMap state.
However, if we switch filters or load more trips, this state gets overridden. So, we need to call updateLocalUnprocessedInputs at some point.
It is an async function but we can call it without 'await' and just allow it to execute in the background.
JGreenlee added a commit to JGreenlee/e-mission-phone that referenced this issue Jan 25, 2024
@JGreenlee
allow labels to be stored as a batch
8161d58 
-- Part of the fix for e-mission/e-mission-docs#1034

When the yellow checkmark is used to verify a pair of inferred mode+purpose labels, both labels should be verified by a single click.
The labels get stored fine; however, the way we update state after doing so prevents both labels from being updated simultaneously and only one of them gets filled in. The resulting UX is that it takes 2 clicks and this is not desired.
We could: update state in succession, one after the other (await label A, then proceed label B),
OR we could rework these functions to support adding labels for a particular trip as a batch (so the same function call could store mode+purpose at the same time). This was decided as a better and more performant option.

It uses Promise.all to store all labels from the batch, then it updates the state with all changes from that batch.
When a single label is recorded, it is a batch of 1 - when verifying inferences, it may be a batch of multiple labels.
JGreenlee added a commit to JGreenlee/e-mission-phone that referenced this issue Jan 25, 2024
@JGreenlee
use memoization pattern for added notes' displayDt
b3785fd 
Instead of mutating an extra property 'displayDt' onto the entries, let's use a get function that returns the displayDt. This should fix issue (4) in e-mission/e-mission-docs#1034.
And let's also memoize that function so performance is not impacted
@github-project-automation github-project-automation bot moved this from Showstoppers for the current release to Tasks completed in OpenPATH Tasks Overview Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
OpenPATH Tasks Overview
Archived in project
Milestone
No milestone
Development

No branches or pull requests
2 participants
@shankari @JGreenlee