Add base mode and footprint to the labeled trips dataframe #152
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
|
I can't see anything obviously wrong. Let me catch up on a few emails and then try this as a separate snippet... |
|
I think the issue is the way I iterate over them, not the way that I insert information. I tried iterating over them and not modifying them, and had the same issue |
this works for meI wonder if the issue is that you are trying to iterate over the iterator twice. Note that |
This was exactly the what I was missing. I converted the iterator to a list, like in your snippet, and things work now. I have successfully added |
the iterator is consumed on first iteration, must use a list to update items instead
|
The data is coming together! There is definitely room for polishing especially in regard to what further unification could take place with
|
having separate columns makes accessing the data easier, no longer need to break a tuple
remove most of the old footprint calculations unpack the footprints into energy and emissions calculate difference between mode and replaced mode
|
After a few more changes - the old charts are working with the new data! |
data function is now async because of waiting on returns from e-mission-common
using older names allows for old code to run
only calculating the footprint when needed in a given notebook saves time
|
The old charts are all working with the new data now: |
updated the version in the build script and updated the calls in scaffolding.py to use the new call pattern some slight data formatting required
|
I still need to make this compatible with dynamic labels! I might need help testing that piece unless I can get my database to work better than it currently is... |
|
@Abby-Wheelis I think I have merged all the dependencies (base mode changes and inferred labels). You should be unblocked to finish up this change. |
|
Not broken after merge! On to trying to rip out the dictionaries so we can pass the labels the same way for all deployments - default or custom per-deployment |
Now relying on the default labels from emcommon
|
Ok, I've removed the csvs now, I think we could further reduce duplication by moving the choice of labels into |
|
For the
All charts are being generated properly for Cortez-ebikes. |
… for program, not study.
|
Testing Stacked Bar Charts for Cortez ebikes with the merged and current changes after ea3cd89: Comparison between Cortez ebikes current and merged
Looks identical. Note: The color variants from base mode appears to be picked up differently each time the charts are being generated. Might be something about dedupe_colors() which needs to be explored. |
|
Testing Scenario: Dataset used: Following notebooks are executed: Detailed execution of above notebooks using `generate_plots.py` script:
Results:
All the charts generation ran smoothly. |
|
Test Scenario: Dataset used: Executed the following notebooks:
Changes in the config file: In Details of execution of the above notebooks:
Could not run the below notebooks: because of the expected error: Results:
All charts generated properly. Note: There is issue with running generic_metrics notebook with dfc-fermata data, it looks up for |
…ated data for generic_metrics noteboook.
Incorporated - Do not execute generic_metrics notebook for survey info. Details
|
I have tested with all three configurations (1), (2), & (3). I have not tallied the data representation in the charts against older version. |
Abby-Wheelis
commented
Sep 24, 2024
viz_scripts/generic_metrics.ipynb
Outdated
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "# Do not run this notebook if it has survey_info; nbclient will run up through this cell\n", |
There was a problem hiding this comment.
We can't filter on if survey info exists -
Some deployments have trip-level surveys: dfc-fermata
And others have records of the onboarding survey: cortezebikes
There was a problem hiding this comment.
Addressed in 468d40e by filtering as
if not survey_info.get('trip-labels', None) == 'MULTILABEL':
\\ Do not execute the generic_metrics notebook.
…TILABEL for generic_metrics notebook
|
Testing Scenario: Dataset: Execution of generate_plots.py for generic_metrics notebook Execution details alongside the git diff for changing `STUDY_CONFIG` and `DB_HOST` in docker-compose.yml :
When running the generate_plots.py for generic_metrics for This addresses this comment #148 (comment) too |
|
Testing Scenario: Dataset: Execution of generate_plots for all the notebooks:
Results:
Stacked Bar Charts issues is being resolved here: #155 Apart from the generation of Stacked Bar Charts issues, everything looks good! |
Abby-Wheelis
commented
Sep 25, 2024
| disp.display(expanded_labeled_inferred_ct.head()) | ||
| return expanded_labeled_inferred_ct | ||
|
|
||
| # CASE 2 of https://github.com/e-mission/em-public-dashboard/issues/69#issuecomment-1256835867 | ||
| unique_users = lambda df: len(df.user_id.unique()) if "user_id" in df.columns else 0 | ||
| trip_label_count = lambda s, df: len(df[s].dropna()) if s in df.columns else 0 | ||
|
|
||
| async def load_viz_notebook_data(year, month, program, study_type, dynamic_labels, dic_re, dic_pur=None, include_test_users=False): | ||
| async def load_viz_notebook_data(year, month, program, study_type, dynamic_labels, include_test_users=False, add_footprint=False): | ||
| #TODO - see how slow the loading the footprint is compared to just the baseMode, and evaluate if passing param around is needed |
There was a problem hiding this comment.
TODO - separate adding base mode and adding footprint?
rather than halt on survey info that does not have multilabel halt on survey info that does have enketo blank survey info means default, which means multilabel, and we should still proceed
|
Looks good! |
|
I have added a few changes -
Now, we can display the CO2 footprint and energy emission even for the ones which uses custom labels. |
|
Test Scenario: Dataset used: Execution of notebooks: Details of execution:
Results:
|
|
Test Scenario: Dataset used: Executed generic_timeseries notebook: Details
Results: |
|
Good idea! Looks good to me |
shankari
approved these changes
Sep 27, 2024
There was a problem hiding this comment.
This was actually a really clean PR and I'm happy to merge it! I think we did a lot of the heavy lifting early with the basemode changes, and so this is so much nicer 😄
I do have a few cleanups that we can roll into a cleanup change later.
At a high level, I think it would be helpful, after we have had a chance to look through what the inferred labels look like, to see if we can have labeled or labeled_versus_inferred energy and emission calculations. Aka actually implement "count every trip". But we should do so on both the dashboard and the phone, so this is a project for CHEER 2025 or similar.
Comment on lines
-429
to
-430
| // Note: We're disabling energy metrics on public dashboard when dynamic labels are available. | ||
| // TODO: Remove the if (data.label_options) in future when energy computation is handled properly. |
There was a problem hiding this comment.
Yay! So happy to handle the TODO!
| @@ -1,4 +1,3 @@ | |||
| 0 7 * * * python bin/update_mappings.py mapping_dictionaries.ipynb >> /var/log/intake.stdinout 2>&1 | |||
There was a problem hiding this comment.
I am so glad that we were able to delete this as well. Fun fact: when people start working with the public dashboard, they would typically not read the instructions to run the mapping_dictionaries first and would report that the run failed.
Glad to see that we won't have that issue any more
| "if survey_info.get('trip-labels', None) == 'ENKETO':\n", | ||
| " ipython = get_ipython()\n", | ||
| " ipython._showtraceback = scaffolding.no_traceback_handler\n", | ||
| " raise Exception(\"The plots in this notebook are not relecant for ENKETO trip-labels\")" |
Comment on lines
+78
to
+79
| except: | ||
| print("hit exception") |
There was a problem hiding this comment.
future fix: I would suggest logging what the exception was (ideally using logging.exception) so that we can check and fix as needed. We may also want to add an entry into the client stats of how many trips were affected. This will allow us to check on why the builds are failing
Comment on lines
+504
to
+516
| def extract_kwh(footprint_dict): | ||
| if 'kwh' in footprint_dict.keys(): | ||
| return footprint_dict['kwh'] | ||
| else: | ||
| print("missing kwh", footprint_dict) | ||
| return np.nan | ||
|
|
||
| def compute_CO2_footprint_dynamic(expanded_ct, dynamic_labels, label_type): | ||
| conversion_meter_to_kilometer = 0.001 | ||
| conversion_kilogram_to_lbs = 2.20462 | ||
| def extract_co2(footprint_dict): | ||
| if 'kg_co2' in footprint_dict.keys(): | ||
| return footprint_dict['kg_co2'] | ||
| else: | ||
| print("missing co2", footprint_dict) | ||
| return np.nan |
There was a problem hiding this comment.
future fix: it seems like this should be combinable into a single parameterized function
Comment on lines
-304
to
+300
| " \"Labeled by user\\n (Confirmed trips)\", ax[1], text_results[1], colors_mode, debug_df_inferred, values_to_translations)\n", | ||
| " \"Labeled and Inferred by OpenPATH\\n\"+stacked_bar_quality_text_commute_inferred, ax[1], text_results[1], colors_mode, debug_df_inferred, values_to_translations)\n", |
There was a problem hiding this comment.
future fix: I think I flagged this somewhere else, but it would be good to change this to "inferred from prior labels" here because there are multiple inference algorithms in OpenPATH
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.
See initial discussion in #146