Closed iantei closed 2 months ago
shankari
commented
2 months ago @iantei Instead of copying over the changes from #143 into a single commit https://github.com/e-mission/em-public-dashboard/pull/148/commits/1808c698e6b8d63a10d5969bd64a79178771fe16 did you:
iantei
commented
2 months ago try pushing to @louisg1337's repo
I tried the following approach:
gh pr checkout 143 to checkout the changes on his PR locally.
Made some changes, and tried to push it to @louisg1337's repo. I do not have write permission to louisg1337/em-public-dashboard.git
ashrest2-35384s:em-public-dashboard ashrest2$ git push
fatal: The upstream branch of your current branch does not match
the name of your current branch. To push to the upstream branch
on the remote, use
git push github-desktop-louisg1337 HEAD:common_colors
To push to the branch of the same name on the remote, use
git push github-desktop-louisg1337 HEAD
To choose either option permanently, see push.default in 'git help config'.
To avoid automatically configuring an upstream branch when its name
won't match the local branch, see option 'simple' of branch.autoSetupMerge
in 'git help config'.
ashrest2-35384s:em-public-dashboard ashrest2$ git push github-desktop-louisg1337 HEAD:common_colors
remote: Permission to louisg1337/em-public-dashboard.git denied to iantei.
fatal: unable to access 'https://github.com/louisg1337/em-public-dashboard.git/': The requested URL returned error: 403if that doesn't work, please pull the commits from his repo as the basis for your PR so that we can have appropriate credit for the changes
Now we have all the commit history from Louis' PR Louis_Changes/common_colors.
iantei
commented
2 months ago Testing:
Dataset: cortezebikes
Tested with the following notebooks:
generate_plots.py worked fine.Below notebook execution threw the following exception:
Exception: The plots in this notebook are only relevant to deployments with trip-level surveys
iantei
commented
2 months ago We would need to handle the case of conversion for "Label" column in "More Info" tab for translated labels. Currently, it shows the internal mode instead of Display mode.
iantei
commented
2 months ago | Before/After | Chart |
|---|---|
| Before | |
| After |
Currently, it shows the internal mode instead of Display mode.
This is resolved.
iantei
commented
2 months ago Comparing the current overall changes in Stacked Bar Charts against one in main branch.
| Main Branch | Charts |
|---|---|
| Main Branch | |
| Current PR changes |
Observations:
The following charts are identical:
e-bike specific trips count by purposee-bike specific number of trips by replaced modee-bike specific trip length (miles) by replaced modeLooks good, but some remapping has been updated.
Miles for each mode -
Total Trip length (miles) covered by mode in land - TODO: Need to filter Air label
We are using internal label instead of Display label. We use other instead of Other for merged labels. Thus, we have other for Sensed labels which are merged as we do not have mapping for Sensed labels.
iantei
commented
2 months ago Total Trip length (miles) covered by mode in land - TODO: Need to filter Air label
Fixed:
| Update | Chart |
|---|---|
| Old | |
| New (Air filtered) |
We have reduced number of labeled trips, and no Air commute present.
iantei
commented
2 months ago TODO: Need to test with dynamic labels.
Abby-Wheelis
commented
2 months ago The filtering of Air modes is also addressed in - #141, so feel free to leave that to my patch or pull my changes, whichever makes more sense to you - I still need to test this PR, but I believe the patches to be mostly done
iantei
commented
2 months ago The filtering of Air modes is also addressed in - https://github.com/e-mission/em-public-dashboard/pull/141, so feel free to leave that to my patch or pull my changes, whichever makes more sense to you - I still need to test this PR, but I believe the patches to be mostly done
The changes to handle Air mode is identical to the way I have handled in this PR. Except, I have used merge_mode_confirm which has collated labels into other which are unavailable in label-options file.
iantei
commented
2 months ago Testing with dynamic_labels:
Dataset used - usaid-laos
Executed for the following notebooks:
usaid-laos is a study.Comparison with current main branch and current PR
| Title | Chart |
|---|---|
| Main branch | |
| Current PR |
All the charts look identical, with an update to the color palette.
Abby-Wheelis
commented
2 months ago
- Looks good, but some remapping has been updated.
Number of trips by purpose
- Shopping, Recreation/Exercise, Home, Entertainment/Social, Personal/Medical have same counts.
- Other (1005) has been split into Other (481) + Access/Recreation (267) + Pickup/Dropoff item (176) + Pickup/ Dropoff person.
- Miles for each mode -
- Other (3032) is split to Other + Air (1306 + 1726)
Total Trip length (miles) covered by mode in land - TODO: Need to filter Air label
- Other (3032) is split to Other(1306) + Air (1726)
We are using internal label instead of Display label. We use
otherinstead ofOtherfor merged labels. Thus, we haveotherfor Sensed labels which are merged as we do not have mapping for Sensed labels.
Can you explain if these changes are expected? I'm a bit confused if we anticipated changes to what was grouped as "Other" in particular. If there is an explanation as to why it was changed then that works for me!
iantei
commented
2 months ago @Abby-Wheelis
In generic_metrics_sensed and survey_metrics notebook -
if bluetooth_only and 'ble_sensed_summary' in expanded_ct.columns:
expanded_ct = expanded_ct[expanded_ct['ble_sensed_summary'].notna()]
print(expanded_ct.ble_sensed_summary.head())
expanded_ct["primary_mode"] = expanded_ct.ble_sensed_summary.apply(lambda md: max(md["distance"], key=md["distance"].get))
unique_keys = expanded_ct.groupby("primary_mode").agg({distance_col: "count"}).index
print(unique_keys)
# TODO:: Needs to be updated
colors_mode, colors_purpose, colors_sensed = await scaffolding.mapping_color_labels({}, dict(zip(unique_keys, unique_keys)), {})
colors_sensed = colors_modeThis specific code for mapping_color_labels() needs to be updated as mapping_color_labels() has been updated to only pass dynamic_labels as the param for the function, and we are no longer using dic_re for mapping for default labels.
I was wondering why we have to pass this list of unique_keys. Looking at this test scenario - https://github.com/e-mission/em-public-dashboard/pull/135#issuecomment-2101582645 the list of primary_mode seems like list of SENSED_MODES?
But then, we are trying to over-ride the color_sensed dict returned from mapping_color_labels().
If we need to pass it as a dictionary, such that we have different values than the BASE_MODE. It would be a good idea to create a new function and handle custom color mapping like with survey_responses.
Abby-Wheelis
commented
2 months ago In the case of bluetooth deployments, we want to see what the primary ble mode of each trip is - in the case of dfc-fermata, this would be CAR, E_CAR, or UNKNOWN. The comment you linked was while this feature was disabled while we were waiting for the data to be ready, this is a more accurate representation of what this needs to look like. They come from the baseMode attribute in the config such as here: dfc fermata config. So they should be base modes, not sensed modes.
iantei
commented
2 months ago Can you explain if these changes are expected? I'm a bit confused if we anticipated changes to what was grouped as "Other" in particular. If there is an explanation as to why it was changed then that works for me!
Yes, these changes are expected of new categorical division of "Other" was expected. In short, this is because of the difference in list of mode/ purpose labels from em-public-dashboard/auxillary_files/'s mode_labels and purpose_labels csv files with the emcommon/label-options.default.json.
Earlier, we were using auxillary_files csv - mode_labels, and purpose_labels to map the internal labels with Display labels. Therefore, whenever we came across internal labels like access_recreation, pick_drop_person and pick_drop_item which we not available in the list of csv; we would aggregate them as "Other". Now, as we have adopted to use label-options.default.json, these aggregation will not happen. Therefore, we will have more distinct labels.
iantei
commented
2 months ago For the bluetooth deployments, I have created a new mapping to return ble specific color mapping in the commit - f1176f6
iantei
commented
2 months ago Testing Scenario:
A. cortezebikes | program, uses label_options from emcommon
Dataset used: cortezebikes
Executed the following notebooks:
| Chart version | Chart |
|---|---|
| Main | |
| Current |
B. usaid-laos-ev | study, uses dynamic_labels from nrel-openpath-deploy-configs
Dataset used: usaid-laos-ev
Executed the following notebooks:
| Chart version | Chart |
|---|---|
| Main | |
| Current |
Conclusion: Everything looks fine.
TODO:: Need test survey_metrics and survey_responses notebook
iantei
commented
2 months ago Test with dfc-fermata | study, no dynamic_labels:
Dataset used: openpath_prod_dfc_fermata_apr_24
Executed the following notebooks:
```
ashrest2-35384s:em-public-dashboard ashrest2$ docker exec -it em-public-dashboard-notebook-server-1 /bin/bash
root@019d62adc912:/usr/src/app# source setup/activate.sh && cd saved-notebooks
(emission) root@019d62adc912:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/update_mappings.py mapping_dictionaries.ipynb
(emission) root@019d62adc912:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py survey_responses.ipynb default
/usr/src/app/saved-notebooks/bin/generate_plots.py:30: SyntaxWarning: "is not" with a literal. Did you mean "!="?
if r.status_code is not 200:
About to download config from https://raw.githubusercontent.com/e-mission/nrel-openpath-deploy-configs/main/configs/dfc-fermata.nrel-op.json
Successfully downloaded config with version 9 for Fermata and data collection URL https://dfc-fermata-openpath.nrel.gov/api/
label_options is unavailable for the dynamic_config in dfc-fermata
Running at 2024-09-16T22:39:21.270806+00:00 with args Namespace(plot_notebook='survey_responses.ipynb', program='default', date=None) for range (
Results:
There seems to be no data which could be displayed.
iantei
commented
2 months ago A few changes in the testing strategy above:
Dataset used - openpath_prod_dfc_fermata_apr_24
Enabled include_test_users = True and updated survey_info = {} from https://github.com/iantei/nrel-openpath-deploy-configs/blob/main/configs/dfc-fermata.nrel-op.json
Following are the results from the notebooks:
survey_metrics notebook
survey_responses notebook - No changes from this current PR
Generic_metrics notebook
Investigating further for Labeled Trip bars, there is no mode_confirm col in the expanded_ct dataframe. Maybe something to do with dataset.
expanded_ct, ... = scaffolding.oad_viz_notebook_data()
expanded_ct.columns
Index(['source', 'end_ts', 'end_fmt_time', 'end_loc', 'raw_trip', 'start_ts',
'start_fmt_time', 'start_loc', 'duration', 'distance', 'start_place',
'end_place', 'cleaned_trip', 'inferred_labels', 'inferred_trip',
'expectation', 'confidence_threshold', 'expected_trip',
'inferred_section_summary', 'cleaned_section_summary', 'user_input',
'additions', 'start_local_dt_year', 'start_local_dt_month',
'start_local_dt_day', 'start_local_dt_hour', 'start_local_dt_minute',
'start_local_dt_second', 'start_local_dt_weekday',
'start_local_dt_timezone', 'end_local_dt_year', 'end_local_dt_month',
'end_local_dt_day', 'end_local_dt_hour', 'end_local_dt_minute',
'end_local_dt_second', 'end_local_dt_weekday', 'end_local_dt_timezone',
'_id', 'user_id', 'metadata_write_ts', 'trip_user_input',
'distance_miles', 'distance_kms']Overall testing with different dataset and notebooks conducted. Looks good!
Abby-Wheelis
commented
2 months ago survey_metrics notebook
Did you make sure that bluetooth_only was true here? This notebook should display the bluetooth detected modes (CAR, E_CAR, UNKOWN`)
Generic_metrics notebook
Right, there are no labels in the survey deployments - just surveys! The survey_metrics and survey_responses replace the other charts. You can take a look at the charts specified here for the survey deployments.
If you want to test on the command line for the survey deployment, you could fork the config repo and set include_test_users to true, then change the download_url to your repo and branch instead of the main branch.
iantei
commented
2 months ago If you want to test on the command line for the survey deployment, you could fork the config repo and set include_test_users to true, then change the download_url to your repo and branch instead of the main branch.
Changes made in docker-compose.yml
-services
- environment:
- DB_HOST=mongodb://db/openpath_prod_dfc_fermata
...
- STUDY_CONFIG=dfc-fermataForked nrel-openpath-deploy-configs and changed the include_test_users = "true" in my forked nrel-openpath-deploy-configs for dfc-fermata. Inside generate_plots.py, changed `download_url = https://raw.githubusercontent.com/iantei/nrel-openpath-deploy-configs/enable_test_users/configs/dfc-fermata.nrel-op.json.
```
(emission) root@0648265f5c09:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/update_mappings.py mapping_dictionaries.ipynb
(emission) root@0648265f5c09:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py survey_metrics.ipynb default
/usr/src/app/saved-notebooks/bin/generate_plots.py:31: SyntaxWarning: "is not" with a literal. Did you mean "!="?
if r.status_code is not 200:
About to download config from https://raw.githubusercontent.com/iantei/nrel-openpath-deploy-configs/enable_test_users/configs/dfc-fermata.nrel-op.json
Successfully downloaded config with version 9 for Fermata and data collection URL https://dfc-fermata-openpath.nrel.gov/api/
label_options is unavailable for the dynamic_config in dfc-fermata
Running at 2024-09-18T04:49:00.489292+00:00 with args Namespace(plot_notebook='survey_metrics.ipynb', program='default', date=None) for range (
Did you make sure that bluetooth_only was true here? This notebook should display the bluetooth detected modes (CAR, E_CAR, UNKOWN`)
Yes
Abby-Wheelis
commented
2 months ago Did you make sure that bluetooth_only was true here? This notebook should display the bluetooth detected modes (CAR, E_CAR, UNKOWN`)
If bluetooth_only is set to true, then the BLE sensed mode should replace the sensed modes for all applicable charts. The charts should look like this, which you can see on production: https://dfc-fermata-openpath.nrel.gov/public/
If we are not able to replace with BLE sensed modes for some reason - like they don't exist or there is an error in the data - then we just need to know why it's not happening. The dump that you loaded (April 24th) could be older than our testing with BLE beacons, I can't recall exactly when that started. What happens when you execute the cell that is meant to replace the sense summaries with BLE sensed summaries?
iantei
commented
2 months ago If we are not able to replace with BLE sensed modes for some reason - like they don't exist or there is an error in the data - then we just need to know why it's not happening. The dump that you loaded (April 24th) could be older than our testing with BLE beacons, I can't recall exactly when that started. What happens when you execute the cell that is meant to replace the sense summaries with BLE sensed summaries?
This is the list of columns which we have in expanded_ct_sensed dataframe:
If we are not able to replace with BLE sensed modes for some reason - like they don't exist or there is an error in the data - then we just need to know why it's not happening. The dump that you loaded (April 24th) could be older than our testing with BLE beacons, I can't recall exactly when that started. What happens when you execute the cell that is meant to replace the sense summaries with BLE sensed summaries?We do not have ble_sensed_summary column in the dataset I used. Since, there is no ble_sensed_summary, it skips.
shankari
commented
2 months ago I have merged https://github.com/e-mission/e-mission-server/pull/981 Starting this review now...
shankari
commented
2 months ago I can make comments and send this back again, but this is blocking the other deliverables, so I am just going to make the changes and merge.
shankari
commented
2 months ago Investigating further for Labeled Trip bars, there is no mode_confirm col in the expanded_ct dataframe. Maybe something to do with dataset.
dfc_fermata is a not a multi-label deployment, so it won't have any confirmed modes. I don't understand why generic_metrics even ran and generated results. In a properly working fleet deployment, all sensed modes should be IN_VEHICLE so it is not clear what the generic metrics tell us
Abby-Wheelis
commented
2 months ago
dfc_fermatais a not a multi-label deployment, so it won't have any confirmed modes. I don't understand whygeneric_metricseven ran and generated results. In a properly working fleet deployment, all sensed modes should beIN_VEHICLEso it is not clear what the generic metrics tell us
None of the charts from that notebook are called for display on the dashboard in fleet deployments or survey deployments, so you're right, we should not run it. It should not break, since we have error handling for missing mode_confirm or missing data in the notebook, but no reason to run it if the charts will never be shown.
I will start an issue to track this issue and that of the survey_metrics vs generic_metrics_sensed so they are a future cleanup separate from this base modes change.
Abby-Wheelis
commented
2 months ago 1) Leave as is (call default labels in each of the 3 data fetching functions in scaffolding.py) - this is slightly cleaned up
2) lift label fetching to generate_plots.py - would mean essentially all labels are dynamic labels (could rename them to simply lables) if they come from either our default or a deployer provided JSON file - what are implications of that on emissions calculations and specific filtering related to things like air or work
3) could call from each notebook - but that is still lots of calls and it will be the same for all the notebooks
I would propose for the sake of time that we merge this PR as-is (label fetching is better, but not perfect) then implement (2) as a part of the wider removal of the csvs and incorporation of the principled energy and emissions calculations in #152
@shankari what do you think of this plan (to do 1, then 2 in the next PR)
iantei
commented
2 months ago I also don't see why the PR contains all of louis's comments AND https://github.com/e-mission/em-public-dashboard/commit/1808c698e6b8d63a10d5969bd64a79178771fe16 which is one giant commit that incorporates all his changes.
I had already created a new PR and made some changes. I had initialized the new PR by taking all of louis's changes. Then I eventually merged his commits into my PR.
iantei
commented
2 months ago Testing with cortezebikes dataset
shankari
commented
2 months ago @Abby-Wheelis the plan is fine. Is this otherwise ready for review?
Abby-Wheelis
commented
2 months ago Yes, it should be, but there seems to be an issue with the emission common version, I am planning to test and see if I can reproduce first thing in the morning to get that resolved
On Thu, Sep 19, 2024 at 10:20 PM K. Shankari @.***> wrote:
@Abby-Wheelis https://github.com/Abby-Wheelis the plan is fine. Is this otherwise ready for review?
— Reply to this email directly, view it on GitHub https://github.com/e-mission/em-public-dashboard/pull/148#issuecomment-2362739432, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANCO3FYMOPBEUUAM3QTXN4DZXOO7BAVCNFSM6AAAAABNU5UOPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRSG4ZTSNBTGI . You are receiving this because you were mentioned.Message ID: @.***>
shankari
commented
2 months ago You may want to check that you are rebuilding so that you pull the most recent version of the server. And maybe use docker exec to access the server container and verify that common was updated correctly.
Abby-Wheelis
commented
2 months ago @iantei I'm still happy to test this, but I would suspect that you just need to pull in the latest commits from main, it looks like you're missing Updated docker image tags in .env files to the latest timestamps
The newest server image is where the new version of e-mission-common is present
Abby-Wheelis
commented
2 months ago Confirmed - pulling in that commit to update the server image tag will give the correct version of e-mission-common
Abby-Wheelis
commented
2 months ago Other than needing to merge main to get the server version bump, this looks good now. I think it will be able to be much cleaner once we can completely remove the csvs
iantei
commented
2 months ago Just merged main and tested the new changes by building generic_metrics and mode_specific_metrics plots through generate_plots.py script. It's working as expected.
Changes copied from Integrate common base mode color into public dashboard #143