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

fix timeline initial date range regression #1162

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

JGreenlee
Copy link
Collaborator

When refactoring into TimelineContext, there was a regression regarding the date range that is initially loaded. #1142 (comment)

It used to load 7 days before the pipelineEnd up to today. After the change, it would load 7 days before today up to today. If the user has travel everyday this ends up being equivalent. But if there isn't recent travel, this becomes a problem.

So in TimelineContext, the initial dateRange needs to depend on pipelineRange. instead of giving an initial value upfront, we'll let dateRange be null until pipelineRange is set, at which point we'll also set dateRange. This means we have to consider some extra cases where dateRange can be null.

When refactoring into TimelineContext, there was a regression regarding the date range that is initially loaded.
e-mission#1142 (comment)

It used to load 7 days before the pipelineEnd up to today. After the change, it would load 7 days before today up to today.
If the user has travel everyday this ends up being equivalent. But if there isn't recent travel, this becomes a problem.

So in TimelineContext, the initial dateRange needs to depend on pipelineRange. instead of giving an initial value upfront, we'll let dateRange be null until pipelineRange is set, at which point we'll also set dateRange.
This means we have to consider some extra cases where dateRange can be null.
@JGreenlee JGreenlee marked this pull request as ready for review June 13, 2024 17:34
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 38.09524% with 13 lines in your changes missing coverage. Please review.

Project coverage is 30.57%. Comparing base (e55e5ae) to head (1955618).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1162      +/-   ##
==========================================
- Coverage   30.59%   30.57%   -0.03%     
==========================================
  Files         118      118              
  Lines        5216     5220       +4     
  Branches     1154     1107      -47     
==========================================
  Hits         1596     1596              
- Misses       3618     3622       +4     
  Partials        2        2              
Flag Coverage Δ
unit 30.57% <38.09%> (-0.03%) ⬇️

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

Files Coverage Δ
www/js/diary/list/DateSelect.tsx 50.00% <100.00%> (ø)
www/js/metrics/MetricsTab.tsx 0.00% <0.00%> (ø)
www/js/TimelineContext.ts 50.30% <38.88%> (-1.27%) ⬇️

@shankari
Copy link
Contributor

shankari commented Jun 25, 2024

So in TimelineContext, the initial dateRange needs to depend on pipelineRange. instead of giving an initial value upfront, we'll let dateRange be null until pipelineRange is set, at which point we'll also set dateRange. This means we have to consider some extra cases where dateRange can be null.

I am going to merge this for now, but I think it would be good to think through what will happen if the network is down and we are not able to retrieve the pipelineRange. Not sure what it does right now, but at some point it used to be able to show draft trips. with this change, I think it will not show anything.

@shankari shankari merged commit a1be7dd into e-mission:master Jun 25, 2024
8 of 9 checks passed
@shankari
Copy link
Contributor

shankari commented Jul 9, 2024

After this fix, things look at lot better. The regressions from #1142 seem to be fixed, there are no errors with aggregate trips. I still don't see the group bar and now I don't see the "worst case". The lack of group bar may just because we don't have any other data for that week, but I don't understand why the worst case is not showing up.

@JGreenlee last fix (if needed!) and then we are ready to move this to production!

Before labeling Before labeling trips Labels After labeling
Simulator Screenshot - iphone 17 Pro - 2024-07-09 at 14 28 22 Simulator Screenshot - iphone 17 Pro - 2024-07-09 at 14 28 35 Simulator Screenshot - iphone 17 Pro - 2024-07-09 at 14 28 49 Simulator Screenshot - iphone 17 Pro - 2024-07-09 at 14 29 21

@shankari
Copy link
Contributor

shankari commented Jul 9, 2024

Note also that bikeshare doesn't seem to count against active minutes

Walk and bikeshare Walk and bike Now bike shows up In the per-day as well
Simulator Screenshot - iphone 17 Pro - 2024-07-09 at 14 39 35 Simulator Screenshot - iphone 17 Pro - 2024-07-09 at 14 40 54 . Simulator Screenshot - iphone 17 Pro - 2024-07-09 at 14 40 58 Simulator Screenshot - iphone 17 Pro - 2024-07-09 at 14 41 05

@shankari
Copy link
Contributor

I checked this on my personal phone, and I see the worst case, but not the group average. I also got a timeout on retrieving the aggregate value the first time I went to the dashboard. @JGreenlee

@shankari
Copy link
Contributor

@JGreenlee example of dashboard (intermittently) not updating

Active modes filled in Not showing in graph or in raw data
Screenshot_2024-07-10-10-09-32-19_5062a5234f99d532322a40d030e8949e Screenshot_2024-07-10-10-10-02-98_5062a5234f99d532322a40d030e8949e Screenshot_2024-07-10-10-10-21-93_5062a5234f99d532322a40d030e8949e

@shankari
Copy link
Contributor

My personal phone is on nrel_commute, so I also upgraded it and then checked the logs. We do get the correct yyyy_mm_dd and it returns 242 results. But I still don't see the comparison in the UI

2024-07-11 05:05:22,499:DEBUG:140319168288320:START POST /result/metrics/yyyy_mm_dd
2024-07-11 05:05:22,499:DEBUG:140319168288320:Aggregate call, checking user_only policy
...
2024-07-11 05:05:22,502:DEBUG:140319168288320:metric_list = {'distance': ['mode_confirm'], 'duration': ['mode_confirm'], 'count': ['mode_confirm']}
2024-07-11 05:05:22,502:DEBUG:140319168288320:for user None, returning timeseries <emission.storage.timeseries.aggregate_timeseries.AggregateTimeSeries object at 0x7f9e835a27f0>
2024-07-11 05:05:22,502:DEBUG:140319168288320:curr_query = {'invalid': {'$exists': False}, '$or': [{'metadata.key': 'analysis/composite_trip'}], 'data.start_fmt_time': {'$lte': '2024-07-10Z', '$gte': '2024-06-26'}}, sort_key = None
2024-07-11 05:05:22,503:DEBUG:140319168288320:orig_ts_db_keys = [], analysis_ts_db_keys = ['analysis/composite_trip']
2024-07-11 05:05:22,503:DEBUG:140319168288320:finished querying values for [], count = 0
2024-07-11 05:06:19,580:DEBUG:140319168288320:finished querying values for ['analysis/composite_trip'], count = 242
2024-07-11 05:06:19,580:DEBUG:140319168288320:orig_ts_db_matches = 0, analysis_ts_db_matches = 242
2024-07-11 05:06:20,806:DEBUG:140319168288320:Returning entry with length 242 result
2024-07-11 05:06:20,854:DEBUG:140319168288320:END POST /result/metrics/yyyy_mm_dd ... 58.35479283332825 

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