-
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
Refactor footprint changes #167
Conversation
…into a single parameterized function extract_footprint(footprint_dict, footprint_key). Removed the print missing kwh/co2, footprint_dict since it was polluting the cell execution, and in all these cases footprint_dict was empty.
…he actual exception error reason, also while accounting the total number of trips where there were errors.
…red from prior labels
…book_inferred_data(,..,add_footprint). Use base_mode AIR to filter out the trip without air mode, instead of internal label
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 have a couple questions/suggestions below!
@@ -244,7 +246,7 @@ | |||
" plot_and_text_stacked_bar_chart(expanded_ct, lambda df: (df.groupby(\"mode_confirm_w_other\").agg({distance_col: 'count'}).sort_values(by=distance_col, ascending=False)), \n", | |||
" \"Labeled by user\\n\"+stacked_bar_quality_text_labeled, ax[0], text_results[0], colors_mode, debug_df, values_to_translations)\n", | |||
" plot_and_text_stacked_bar_chart(expanded_ct_inferred, lambda df: (df.groupby(\"mode_confirm_w_other\").agg({distance_col: 'count'}).sort_values(by=distance_col, ascending=False)), \n", | |||
" \"Labeled and Inferred by OpenPATH\\n\"+stacked_bar_quality_text_inferred, ax[1], text_results[1], colors_mode, debug_df_inferred, values_to_translations)\n", | |||
" \"Inferred from prior labels\\n\"+stacked_bar_quality_text_inferred, ax[1], text_results[1], colors_mode, debug_df_inferred, values_to_translations)\n", |
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.
Good text change. More concise & clearer
Co-authored-by: Jack Greenlee <[email protected]>
Co-authored-by: Jack Greenlee <[email protected]>
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!
trip['data']['base_mode'] = "UNKNOWN" | ||
trip['data']['replaced_base_mode'] = "UNKNOWN" | ||
trip['data']['mode_confirm_footprint'] = {} | ||
trip['data']['replaced_mode_footprint'] = {} | ||
logging.debug(f"There are {counter_trip_error} trip errors") |
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.
Interesting. As a future cleanup, I think we should report these errors in some place other than logs @Abby-Wheelis @iantei so we don't have to scrape logs to figure out if there is something we need to fix.
Maybe in the server stats, or even displayed in the generated chart if we can figure out a way to do so without confusing people too much.
@shankari Merge conflicts has been resolved, and tested. Testing Scenario: Dataset used: Detailed execution of notebooks with generate_plots.py scripts:
Changes look good! |
Handle Future cleanups mentioned in #152 Changes:
Type fixed from relecant to relevant.
Refactor extract_co2 and extract_kwh to a single consolidated parameterized function extract_footprint()