-
Notifications
You must be signed in to change notification settings - Fork 23
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
Remove display labels #166
base: main
Are you sure you want to change the base?
Conversation
iantei
commented
Oct 10, 2024
- 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().
… 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
…labels(labels). Removed merged conflict issue remanant.
Testing Scenario: Dataset used: Detailed execution of notebook scripts using `generate_plots.py`:
Looks good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Screenshots look great, nice to see all the changes over the last year come together nicely. I have a couple minor comments/ points of clarification
if values_to_translations: | ||
x_ticks = ax.get_xticklabels() | ||
translated_labels = [values_to_translations.get(label.get_text(), label.get_text()) for label in x_ticks] | ||
ax.set_xticklabels(translated_labels) | ||
|
||
ax.set_xticklabels(ax.get_xticklabels(), rotation=45, ha='right') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, the translations are only applied here, during the creation of the plot.
Meaning that everywhere else it is internally using the value, which is good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was initially thrown off by
values_to_translations.get(label.get_text(), label.get_text()
but I understand now.
label.get_text()
is the "value" and we are searching values_to_translations
for a mapping. But if that mapping doesn't exist (like with 'other' or a custom mode or something) then it defaults to the raw value
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused by y.map
until I realized y
is a Series
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |