Fix: for labeled and labeled & inferred bars generation broken for ca-ebike
#155
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
…n the label-options.
|
Test Scenario: Dataset: Execution of `generic_metrics` and `mode_specific_metrics` notebook using `generate_plots.py` script:
Result:
Looks good! |
|
Regression testing with Detailed execution of `generic_metrics` and `mode_specific_metrics` notebook via. `generate_plots.py` script:
Results:
Looks good! |
|
It looks like this fixes the colors, but breaks the labels, the translation provided to us is |
|
We talked and @iantei pointed out that there were no trips labeled I still think it would be best to have it show as |
Abby-Wheelis
approved these changes
Sep 25, 2024
I am not sure what "they should show as other, not Other mode" means but "they are mostly labeled before the custom labels were implemented) is incorrect. In the default options, there is no "other" option. If the user selects "other" then they are prompted to enter a custom mode. So we will never see "other" as a mode_confirm with the default values, unless the user types out "other" which is unlikely. The CA e-bike team did not want this behavior - they wanted the user to be able to enter "other" with a single click and not have to type out a mode. So they intentionally created a custom mode called "other_mode" so that the other handling from the UI would not kick in. So this "other" should be handled just like any other mode that is not in the default list. |
| @@ -296,6 +296,11 @@ async def mapping_color_labels(dynamic_labels = {}, unique_keys = []): | |||
| 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 [] | |||
|
|
|||
| # Append 'other' to the list if not present | |||
There was a problem hiding this comment.
Wait, so what makes this different than assigning the labels not in the list as "other" on line 203 of this file. Why does this come up specifically for ca-ebike?
There was a problem hiding this comment.
In line 203, we are binning the internal labels which are not available on the label-options to 'other'. This is being called from load_viz_notebook_data(), while the above case is added for color mapping which is created before the call to load_viz_notebook_data().
This comes specifically for ca-ebike because we do not have 'other' in ca-ebike label-options json file.
|
I think we might both be saying the same thing: for this deployment, if the label selected or input by the user is not in the list, then we should display it as "Other" like we do for any other label NOT in the list. If the user selects "Other mode" then we should display that like we do any other label that IS in the list. Now, the part that I don't understand is why this is specific to Ah, but So if this addition of So, we need to add In that case, can we go ahead and manually add it to the mapping dictionary too? |
|
I have explained the issue in details here (#154 (comment)) |
This pretty much aligns with the current design of how any other mode that is not in the default list is being handled. It is being binned to "other". However, this is a different case too, because we do not have “other” in the label-options for “ca-ebike”. Here, the case of “other” is a label which is used to bin all small merged labels. So, categorically, it is not present in the mode_confirm, purpose_confirm, or replaced_mode list. Why were we not facing this issue earlier? Use of Display labels instead of internal labels, and whenever, we didn’t had a label which suit our needs would be transformed into Other. Thus represented as “Other” in the chart’s label. Therefore, we did not need translation mapping. For all the current cases, except “ca-ebike” program (for now), we have an en-translation either in the default-label-options from e-mission-common, or from the program’s deployment specific label-options which maps “other” -> “Other” Should we represent it as “Other”, which in visualization sense makes sense, but in accordance to how we map the internal labels to the Display labels using the en-translation does not quite match? Also, it should be noted that, since we do not have - “other” - > baseMode: “OTHER”, we are considering the baseMode for “other” as de_dupe(“UNKNOWN”) which is the default baseMode. |
|
If we plan to handle the "other" to showcase as "Other" in the dashboard: I see two approaches in which we can handle this:
|
|
Testing Scenario (Testing with change in commit 696fe87): Dataset used: Execution of generic_metrics and mode_specific_metrics notebooks:
Result: 
"other" is transformed to "Other" |
…key is unavailable
|
Regression testing with Cortezebikes Dataset used : Execution of notebooks: generic_metrics and mode_specific_metrics
Results: 
All Stacked Bar Charts are generated properly. |
|
I am not opposed to this change since it improves robustness and corner case handling. However, I just want to clarify the expectation and terminology
I think that by "internal labels" @iantei means the labels that are not defined in the deployment labels (either default or custom). I don't think that is a great name. These labels are the exact opposite of internal, they are not created by anybody at NREL or at our deployment partner, instead, they are created by users who hit "other" and then typed out a new value. I would suggest participant-defined labels for clarity.
I think @iantei has explained this - in this case, I also want to highlight that the goal of the CA e-bike program was to ensure that there would not be any participant-specified labels. They did not want to allow participants to enter random labels and then have to unify them. Since the functionality to support participant labels is tied to the participant selecting "other" from the dropdown, they chose to not have an "other" mode in their list of supported modes. This means that there should not be any participant-specified labels in the ca-ebike dataset. That is what I am trying to highlight here: #155 (comment) This issue should have never come up in the first place because unless there is an "other" in the configured list of modes for the deployment, users will not be able to enter their own modes. I suspect that the CA e-bike deployment is currently broken because they used to have the "other" mode when they were testing their configuration, before they created their customized configuration. But all that data should be under "test" users, and we should be free to delete all existing data for test users to avoid this. To verify this, we can simply turn off the @iantei if it will not take more than an hour, I would like to see you verify that. Given that, do we still need this improvement? We should not see participant-defined entries unless there was an other in the configuration, and if there is an other in the configuration, this should never happen. |
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.
More details about the issue: #154
Add
otherto the list of mode/purpose_replace_values if not present in the label-options.