Remove display labels #166
Open
+86
−86
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
iantei
commented
Oct 10, 2024
iantei
commented
- We have removed the auxillary_files/ *.csv files, and now we are dependent on the usage of label-options file, either provided by the program deployment or from the e-mission-common's default-label-options.
- Removing the dependency on the Display labels.
- Usage of internal labels for the creation of bars.
…k_data() for scaffolding.py
…xpanded_ct dataframe.
…I(kWH), Replaced_mode_kg/lb_CO2 with mode_confirm_EI(kWH), mode_confirm_kg/lb_CO2, replaced_mode_EI(kWH) and replaced_mode_kg/lb_CO2 respectively.
…display labels like Mode_confirm column in expanded_ct dataframe.
…notebook. Deprecate the use of display labels like Mode_confirm column in expanded_ct dataframe.
… mode_distance_interest as we do not use Mode_confirm - in mode_specific_timeseries notebook.
…ted trips for Generic Metrics notebook
…slations_replaced as an additional param to energy_impact and CO2_impact plot creation function in plots.py. Map values_to_translations to the respective replaced mode labels in plots.py for energy_impact and CO2_impact.
…ion from internal labels to en-translated version from label-options. 2. Change the filter to use internal label instead of Display labels. 3. Pass key of MODE's from label-option as the categorical list. 4. Remove unnecessary mapping.
…mpact(), CO2_impact() and timeseries_multi_plot() for translations from internal labels to en-version of translation. Set up the y_labels for energy_impact() and CO2_impact(). Set up labels for timeseries_multi_plot().
…ernal label). Pass values_to_translations as an extra argument in barplot_mode().
|
Testing Scenario: Dataset used - Detailed execution of notebooks:
Results:
Generation of all charts look good |
… for translation of internal labels.
|
Testing Scenario: Dataset used - Execution of notebooks:
Results:
All the charts loaded properly. |
…m value_to_translations_purpose/replaced to values_to_translations_purpose/replaced
|
Changes in the X-axis labels to show proper labels. |
…labels(labels). Removed merged conflict issue remanant.
|
Testing Scenario: Dataset used: Detailed execution of notebook scripts using `generate_plots.py`:
Looks good! |
JGreenlee
approved these changes
Jan 8, 2025
viz_scripts/plots.py
Outdated
| color = color.map({True: 'green', False: 'red'}) | ||
| objects = ('Energy Savings', 'Energy Loss') | ||
|
|
||
| y_labels = y | ||
| y_labels = y.map(values_to_translations) |
There was a problem hiding this comment.
I was confused by y.map until I realized y is a Series
There was a problem hiding this comment.
follow-up question: What happens if one of the translations is missing? Is there potential for that to happen here or other places we do the mapping like this?
If so, could do y.map(lambda x: values_to_translations.get(x, x))
There was a problem hiding this comment.
That's a good catch!
I have incorporated the fall back in other places where we do mapping like this, where we fall back to pick up the raw value whenever translations is unavailable.
E.g. values_to_translations.get(label, label)
I have incorporated this change for both energy_impact and CO2_impact plots.
…exist for mapping to y in CO2_impact and energy_impact plot creation in plots.py.
|
Testing Scenario: Dataset used: Detailed execution of generic_metrics notebook and energy_calculations notebook (Since only these two notebooks were changed after the resolution of merge conflict and last commit 7ebc129 ): Result:
Looks good! |
shankari
approved these changes
Mar 8, 2025
viz_scripts/generic_metrics.ipynb
Outdated
Comment on lines
388
to
389
| " labeled_u80_quality_text = f\"{len(expanded_ct_u80)} trips ({round(len(expanded_ct_u80)/len(expanded_ct)*100)}% of all labeled,\\n{round(len(expanded_ct_u80)/len(expanded_ct_sensed)*100)}% of all trips)\\nfrom {scaffolding.unique_users(expanded_ct_u80)} {sensed_match.group(3)}\" if \"mode_confirm_w_other\" in expanded_ct.columns else \"0 labeled trips\"\n", | ||
| " inferred_u80_quality_text = f\"{len(expanded_ct_inferred_u80)} trips ({round(len(expanded_ct_inferred_u80)/len(expanded_ct_inferred)*100)}% of all inferred,\\n{round(len(expanded_ct_inferred_u80)/len(expanded_ct_sensed)*100)}% of all trips)\\nfrom {scaffolding.unique_users(expanded_ct_inferred_u80)} {sensed_match.group(3)}\" if \"mode_confirm_w_other\" in expanded_ct_inferred.columns else \"0 inferred trips\"\n", |
There was a problem hiding this comment.
Aren't these already defined in lines 386 and 387 just above? Why do you need to re-define them?
There was a problem hiding this comment.
That's a good catch! I have removed the re-definition of these.
| "expanded_ct = scaffolding.unpack_energy_emissions(expanded_ct) if \"mode_confirm_footprint\" in expanded_ct.columns else expanded_ct" | ||
| "expanded_ct = scaffolding.unpack_energy_emissions(expanded_ct) if \"mode_confirm_footprint\" in expanded_ct.columns else expanded_ct\n", | ||
| "\n", | ||
| "values_to_translations, value_to_translations_purpose, values_to_translations_replaced = scaffolding.translate_values_to_labels(labels)" |
There was a problem hiding this comment.
nit, future fix: why is the mode mapping called values_to_translations, while the others have the type of translation (e.g. values_to_translations_purpose)?
There was a problem hiding this comment.
It's a good point to consider renaming this to values_to_translations_mode.
The likely reasoning was -
- Trying to maintain shorter variable name
- Implicit understanding that, mode mapping is the default one over purpose and replaced mode. Now, looking back at this, adding mode suffix makes more sense.
| @@ -115,14 +115,14 @@ | |||
| "\n", | |||
| "if use_imperial:\n", | |||
| "# sel_cols_no_label_dep = ['user_id','start_local_dt_year','start_local_dt_month','start_local_dt_day','distance_miles']\n", | |||
| " sel_cols_with_label_dep = sel_cols_no_label_dep + ['Mode_confirm','Mode_confirm_EI(kWH)','Mode_confirm_lb_CO2']\n", | |||
| " sel_cols_with_label_dep = sel_cols_no_label_dep + ['mode_confirm_w_other','mode_confirm_EI(kWH)','mode_confirm_lb_CO2']\n", | |||
There was a problem hiding this comment.
nit, future fix: given that we are going to values and not labels, why do we still need to have (kWH) and lb_CO2 in these column names? Doesn't EI imply (kWH)?
There was a problem hiding this comment.
I couldn't understand - what you meant by we are going to values and not labels?
- We are supporting both Metric and Imperial units for weight - we have lb and kg representation of CO2.
- EI could have other units, like
Joule. I feel having the unit as suffix gives better clarity
|
Tested with Testing Scenario:
Results:
While I observed the values for these charts to be different from the previously recorded dataset for Comparison:
Both looks identical. |
|
Note: There is issue while running |
|
Testing with Cortez Ebikes - without dynamic labels Testing Details:
Results:
All charts generated fine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.