Integrate common base mode colors into public dashboard #148

iantei · 2024-09-04T18:50:33Z

Changes copied from Integrate common base mode color into public dashboard #143

…ted the base mode colors into the plots

…amic labels

…olors, and added in emcommon as a dependency

… being because of blocker. Removed unused imports and function. Changed dedupe colors to support new parameters.

…ad of display values.

… into public dashboard e-mission#143.

…tead of en translation mapping.

…s. Introduce values_to_translations_purpose and return it.

…tion parameter. Use purpose_confirm instead of Trip_purpose to create plot_and_text_stacked_bar_chart for Number of trips for each purpose stacked bar chart.

shankari · 2024-09-07T18:04:34Z

@iantei Instead of copying over the changes from #143 into a single commit 1808c69
did you:

try pushing to @louisg1337's repo
if 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

iantei · 2024-09-10T19:18:22Z

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: 403

…rate_base_mode_colors

iantei · 2024-09-10T21:55:26Z

if 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.

…wait read_json_resource(). 2. Introduce merge_ column in the expanded_ct dataframe. 3. Update value_to_basemode default value to UNKNOWN. 4. Create new dictionary for colors_mode and colors_replaced. 5. Return translations for mode, purpose and replaced individually for legend values.

…lay mode.

…es. Update load_viz_notebook_data() to use await. Update use of merge_purpose_confirm over purpose_confirm as column name for Purpose related trip.

…le. Update load_viz_notebook_data() to use await. Update the column passed as Trip_purpose to merge_purpose_confirm and Replaced_mode to merge_replaced_mode for Stacked Bar Charts.

…iables.

…now an async function.

iantei · 2024-09-12T15:52:09Z

Testing:

Dataset: cortezebikes

Tested with the following notebooks:

generic_metrics notebook
generic_metrics_sensed
generic_timeseries
mode_specific_metrics
mode_specific_timeseries
energy_calculations
All the above notebooks execution of 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
7. survey_responses
8. survey_metrics

iantei · 2024-09-12T15:55:35Z

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.

Abby-Wheelis

Other than handling the e-mission-common version centrally in the server instead of in the dashboard (once something like this can be merged) LGTM!

shankari · 2024-09-18T20:56:13Z

I have merged e-mission/e-mission-server#981 Starting this review now...

shankari · 2024-09-19T01:19:19Z

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

I was going to just make the changes myself, and I made one, but there are way more changes that are required here. The multiple retrievals of the default JSON file should be fixed before merge; the others can be cleaned up later.

I have other tasks to complete, but if this is not addressed by the weekend, I guess I can try to set aside some time to work on it then.

I also don't see why the PR contains all of louis's comments AND 1808c69 which is one giant commit that incorporates all his changes.

shankari · 2024-09-19T01:22:02Z

viz_scripts/scaffolding.py

+from emcommon.diary.base_modes import BASE_MODES, dedupe_colors
+from emcommon.util import read_json_resource


Future fix: Our coding standard for python is import X as x and then x.function. We don't use from X import y

Changed to following the coding standard for python.

shankari · 2024-09-19T01:23:19Z

viz_scripts/scaffolding.py

        else:
            expanded_ct['Mode_confirm'] = expanded_ct['mode_confirm'].map(dic_re)
+            labels = await read_json_resource("label-options.default.json")


Future fix: why are we reading the json file twice instead of reading once and re-using. This potentially has network implications.

Fixed it. Calling it at one place now. 0034d6f

shankari · 2024-09-19T01:23:40Z

viz_scripts/scaffolding.py

            else:
                expanded_ct['Replaced_mode'] = expanded_ct['replaced_mode'].map(dic_re)
+                labels = await read_json_resource("label-options.default.json")


This is the other place where we use it.

Fixed it. Calling it just once.

shankari · 2024-09-19T01:24:27Z

viz_scripts/scaffolding.py

+            labels = await read_json_resource("label-options.default.json")
+        # If the 'mode_confirm' is not available as the list of keys in the dynamic_labels or label_options.default.json, then, we should transform it as 'other'
+        mode_values = [item['value'] for item in labels['MODE']]
+        expanded_ct['merge_mode_confirm'] = expanded_ct['mode_confirm'].apply(lambda mode: 'other' if mode not in mode_values else mode)


nit: why is this called merge_XXX? merge generally refers to merging two tables or dataframes together, which is not what is happening here.

We already had mode_confirm column. We are merging the rows which had internal labels which aren't present in the label-options's MODE keys into other.

Would something like grouped_XX or processed_XX be any more clear?

@Abby-Wheelis I would rename it to mode_confirm_w_other as discussed?

Just to clarify, if I say that something is a nit, it doesn't need to hold up the merge. https://en.wikipedia.org/wiki/Nitpicking I would suggest focusing on the important issues - e.g.

The multiple retrievals of the default JSON file should be fixed before merge; the others can be cleaned up later.

and clean the others up later.

This will ensure that we don't make big changes, which have the potential to break code, while making progress towards the merge.

shankari · 2024-09-19T01:25:03Z

viz_scripts/scaffolding.py

        else:
            expanded_ct['Trip_purpose'] = expanded_ct['purpose_confirm'].map(dic_pur)
+            labels = await read_json_resource("label-options.default.json")


Yet another read of the same file

Handled it in this commit - 0034d6f
Created an if/else block to see if there is dynamic_labels or not, and assign it to labels accordingly.

shankari · 2024-09-19T01:26:26Z

viz_scripts/scaffolding.py

-def mapping_color_labels(dynamic_labels, dic_re, dic_pur):
+async def mapping_color_labels(dynamic_labels = {}, unique_keys = []):
+    # Load default options from e-mission-common
+    labels = await read_json_resource("label-options.default.json")


yet another read of the same file

shankari · 2024-09-19T01:37:24Z

viz_scripts/scaffolding.py

@@ -191,28 +207,63 @@ def translate_labels(labels):
                translation = translations.get(value)
                translation_mapping[value] = translation
            return defaultdict(lambda: 'Other', translation_mapping)
-        dic_mapping = translate_labels(dynamic_labels[label_type])
+        dic_mapping = translate_labels(dynamic_labels.get(label_type, ''))


I see that this is added in iantei@ed0c4c6 but I don't see how it works.

Isn't the return value for dynamic_labels.get(label_type) a dictionary with the internal modes as keys? How can the default value returned by get be a string in that case?

Why was this added? How was it tested?

No, the return value for dynamic_labels.get(label_type) is a list of dictionaries.

{ "MODE": [ {"value":"walk", "baseMode":"WALKING", "met_equivalent":"WALKING", "kgCo2PerKm": 0}, ... ], ... }

The default return value should be an empty list instead. But I might have overcomplicated things by adding default value.

Why was this added?

I added this because I ran the usaid-laos-ev study for mode_specific metrics while trying to debug few things. I didn't change the study_type to program. Since, the usaid-laos-ev does not have REPLACED_MODE, I tried to handle the case with default value. when label_type is not present.

How was it tested?

I ran the execution on notebook itself, which resulted in no issue - while showing an empty chart for Replaced_mode related charts.

Should I revert the change to dynamic_labels[label_type] or would returning a default empty list would be a better approach?

The USAID laos project is a study and does not have a mode of interest. So the program specific metrics notebook should error out early and displaying the replaced mode should never be called. I don't understand why this is needed and I am going to remove it before merge. We can discuss re-adding it in the cleanup PR.

Removing it before merge will not help unless we squash merge. But if we squash merge, we cannot appropriately credit the two authors.

Going to merge without squash, but IMHO in the future we should err on the side of avoiding unnecessary changes.

shankari · 2024-09-19T01:44:26Z

viz_scripts/survey_metrics.ipynb

+    "        colors_mode, colors_replaced, colors_purpose, colors_sensed, colors_ble = await scaffolding.mapping_color_labels(unique_keys)\n",
+    "        colors_sensed = colors_ble\n",
+    "except ValueError as e:\n",
+    "    print(\"Got ValueError \", e)"


I am curious why this code block is duplicated in both survey_metrics and generic_metrics_sensed. Surely we need to compute ""#if fleet, replace primary_mode with primary_ble_sensed mode" only once.

shankari · 2024-09-19T04:36:26Z

viz_scripts/scaffolding.py

+    purpose_values = [mode["value"] for mode in labels["PURPOSE"]] if "PURPOSE" in labels else []
+    replaced_values = [mode["value"] for mode in labels["REPLACED_MODE"]] if "REPLACED_MODE" in labels else []
+
+    # Mapping between mode values and base_mode OR baseMode (backwards compatibility)


I'm confused; I see that there is a new function, but it is not invoked here to do the mapping. I see the function invoked in the notebooks and the results passed in to the plots, but not here in scaffolding. If this is no longer relevant, the comment should be removed.

shankari · 2024-09-19T04:43:05Z

viz_scripts/energy_calculations.ipynb

@@ -106,7 +106,7 @@
   },
   "outputs": [],
   "source": [
-    "expanded_ct, file_suffix, quality_text, debug_df = scaffolding.load_viz_notebook_data(year,\n",
+    "expanded_ct, file_suffix, quality_text, debug_df = await scaffolding.load_viz_notebook_data(year,\n",


don't we need the values_to_translations here? we should use the display modes in the energy and emission impact plots.

We do not need the values_to_translations() here. It's being used only with the Stacked Bar Charts. We are using display modes in the energy and emission impact plots.

Wait, so we have mode_confirm, mode_confirm_w_other and Mode_confirm all at the same time? When we remove the csvs I think we should probably get rid of Mode_confirm and just use values_to_translations to prevent confusion.

shankari · 2024-09-19T06:00:59Z

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 · 2024-09-19T14:19:33Z

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

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.

…r label-options.default.json

…i/em-public-dashboard into Integrate_base_mode_colors

Abby-Wheelis · 2024-09-19T21:47:03Z

Leave as is (call default labels in each of the 3 data fetching functions in scaffolding.py) - this is slightly cleaned up
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
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)

…w_other.

…trics notebook.

iantei · 2024-09-19T22:20:20Z

I also don't see why the PR contains all of louis's comments AND 1808c69 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.

…mmon.util to emcu.

iantei · 2024-09-19T23:13:48Z

Testing with cortezebikes dataset

Generation of generic_metrics and mode_specific_metrics notebooks both are good

shankari · 2024-09-20T04:19:39Z

@Abby-Wheelis the plan is fine. Is this otherwise ready for review?

Abby-Wheelis · 2024-09-20T04:37:32Z

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 <#148 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANCO3FYMOPBEUUAM3QTXN4DZXOO7BAVCNFSM6AAAAABNU5UOPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRSG4ZTSNBTGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

shankari · 2024-09-20T05:36:17Z

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 · 2024-09-20T12:36:12Z

@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 · 2024-09-20T13:00:26Z

Confirmed - pulling in that commit to update the server image tag will give the correct version of e-mission-common

Abby-Wheelis · 2024-09-20T13:16:12Z

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 · 2024-09-20T15:27:30Z

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.

shankari · 2024-09-21T05:28:39Z

viz_scripts/scaffolding.py

@@ -153,10 +168,12 @@ def load_viz_notebook_data(year, month, program, study_type, dynamic_labels, dic
    # CASE 2 of https://github.com/e-mission/em-public-dashboard/issues/69#issuecomment-1256835867
    if dic_pur is not None and "purpose_confirm" in expanded_ct.columns:
        if (len(dynamic_labels)):
-             dic_purpose_mapping = mapping_labels(dynamic_labels, "PURPOSE")
-             expanded_ct['Trip_purpose'] = expanded_ct['purpose_confirm'].map(dic_purpose_mapping)
+            dic_purpose_mapping = mapping_labels(dynamic_labels, "PURPOSE")


I assume this will be removed when the csvs are removed

shankari · 2024-09-21T05:34:20Z

viz_scripts/generic_metrics.ipynb

@@ -90,7 +90,8 @@
   "metadata": {},
   "outputs": [],
   "source": [
-    "colors_mode, colors_purpose, colors_sensed = scaffolding.mapping_color_labels(dynamic_labels, dic_re, dic_pur)"
+    "colors_mode, colors_replaced, colors_purpose, colors_sensed, colors_ble = await scaffolding.mapping_color_labels(dynamic_labels)\n",
+    "values_to_translations, value_to_translations_purpose, values_to_translations_replaced = await scaffolding.translate_values_to_labels(dynamic_labels)"


nit: why isn't this values_to_translations_mode?

louisg1337 and others added 14 commits August 8, 2024 15:31

Added in support for emission common to public dashboard, and integra…

58d7697

…ted the base mode colors into the plots

Added in edge case to deal with bike to BICYCLE

ee7cc2e

Removed emcommon dependency install and adjusted code to work for dyn…

25259d2

…amic labels

Added in support for lighter colors if multiple of the same color appear

7b456a9

Use exponential decay coefficient to account for infinite duplicate c…

7137a01

…olors, and added in emcommon as a dependency

Import and use emcommon's dedupe_colors function

6630214

Added support for emcommon default labels, but commented out for time…

998a725

… being because of blocker. Removed unused imports and function. Changed dedupe colors to support new parameters.

Refactored plots and colors so they work off of internal values inste…

ff703a9

…ad of display values.

Initial commit. Changes copied from Integrate common base mode colors…

1808c69

… into public dashboard e-mission#143.

Check for baseMode as key in dynamic_labels

3094859

Update purpose-value mapping from label_options's Purpose's value ins…

86f3798

…tead of en translation mapping.

Remove find_closest_key() since we are not using it anymore.

e2cd09c

Remove dic_re, dic_pur as function parameter from mapping_color_label…

b07ffd1

…s. Introduce values_to_translations_purpose and return it.

Update mapping_color_labels() to only take dynamic_labels as the func…

2e2bedc

…tion parameter. Use purpose_confirm instead of Trip_purpose to create plot_and_text_stacked_bar_chart for Number of trips for each purpose stacked bar chart.

Abby-Wheelis mentioned this pull request Sep 6, 2024

Patches: nan summaries breaking some deployments, filter land modes on underlying (not display) label #141

Merged

Merge remote-tracking branch 'Louis_Changes/common_colors' into Integ…

d4cbcc1

…rate_base_mode_colors

iantei added 9 commits September 10, 2024 23:02

Update Other to other since we pass the internal mode instead of Disp…

8c8b7ff

…lay mode.

Update mapping_color_labels() to use await,and update its return valu…

1ef4c3b

…es. Update load_viz_notebook_data() to use await. Update use of merge_purpose_confirm over purpose_confirm as column name for Purpose related trip.

Update mapping_colors_labels() to use await, and update return variab…

ac65528

…le. Update load_viz_notebook_data() to use await. Update the column passed as Trip_purpose to merge_purpose_confirm and Replaced_mode to merge_replaced_mode for Stacked Bar Charts.

Change the mapping_color_labels() to use await, and update return var…

fccc53d

…iables.

Update use of merge_mode_confirm over mode_confirm.

a0fae22

Update await to scaffolding.load_viz_notebook_data()

12d4795

Remove find_closest_key() since it's not being used anywhere.

51eb1da

Add await in front of scaffolding.mapping_color_labels() since it is …

a5180c4

…now an async function.

Abby-Wheelis approved these changes Sep 18, 2024

View reviewed changes

iantei mentioned this pull request Sep 18, 2024

Change metrics.include_test_users:true e-mission/nrel-openpath-deploy-configs#121

Closed

Remove e-mission-common here since it is pulled from the server

b3a0391

shankari requested changes Sep 19, 2024

View reviewed changes

Abby-Wheelis mentioned this pull request Sep 19, 2024

🧹 ✈️ 👤 Pending cleanups #149

Open

4 tasks

iantei added 3 commits September 19, 2024 10:23

Aggregate a single place to chose labels from either dynamic_labels o…

0034d6f

…r label-options.default.json

Merge branch 'Integrate_base_mode_colors' of https://github.com/iante…

669d521

…i/em-public-dashboard into Integrate_base_mode_colors

Enforce coding standard for python as import X as x and then x.function.

afaed09

iantei added 2 commits September 19, 2024 15:06

Rename the column in expaneded_ct in scaffolding from merge_** to **_…

9cefa33

…w_other.

Rename merge_** to **_w_other in generic_metrics and mode_specific_me…

ec355ca

…trics notebook.

iantei added 2 commits September 19, 2024 15:22

Remove unnecessary conversion of defaultdict to dict.

821fe7d

Rename edb for emcommon.diary.base.modes to emcdb. Rename eu for emco…

ac07f67

…mmon.util to emcu.

Merge branch 'main' into Integrate_base_mode_colors

3cbd76b

shankari approved these changes Sep 21, 2024

View reviewed changes

shankari merged commit de20f12 into e-mission:main Sep 21, 2024

iantei mentioned this pull request Sep 24, 2024

Add base mode and footprint to the labeled trips dataframe #152

Merged

		from emcommon.diary.base_modes import BASE_MODES, dedupe_colors
		from emcommon.util import read_json_resource

Integrate common base mode colors into public dashboard #148

Integrate common base mode colors into public dashboard #148

Conversation

iantei commented Sep 4, 2024

shankari commented Sep 7, 2024

iantei commented Sep 10, 2024

iantei commented Sep 10, 2024

iantei commented Sep 12, 2024

iantei commented Sep 12, 2024

Abby-Wheelis left a comment

Choose a reason for hiding this comment

shankari commented Sep 18, 2024

shankari commented Sep 19, 2024

shankari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iantei Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shankari commented Sep 19, 2024

Abby-Wheelis commented Sep 19, 2024

Abby-Wheelis commented Sep 19, 2024 • edited Loading

iantei commented Sep 19, 2024

iantei commented Sep 19, 2024

shankari commented Sep 20, 2024

Abby-Wheelis commented Sep 20, 2024 via email

shankari commented Sep 20, 2024

Abby-Wheelis commented Sep 20, 2024

Abby-Wheelis commented Sep 20, 2024

Abby-Wheelis commented Sep 20, 2024

iantei commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iantei Sep 19, 2024 •

edited

Loading

Abby-Wheelis commented Sep 19, 2024 •

edited

Loading