-
-
Notifications
You must be signed in to change notification settings - Fork 489
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
Reverse map discrete options using identity #305
Conversation
This seems like a more straightforward approach. Are there downsides? Is it slower? |
Sounds good. Should Param do the same for selectors? |
I doubt it's much slower, if anything I'd expect it to be cheaper not to do the hashing. That said the approach isn't quite right since |
Yes, the |
Codecov Report
@@ Coverage Diff @@
## master #305 +/- ##
==========================================
- Coverage 89.3% 89.24% -0.07%
==========================================
Files 63 63
Lines 6528 6565 +37
==========================================
+ Hits 5830 5859 +29
- Misses 698 706 +8
Continue to review full report at Codecov.
|
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
Using a dictionary to map options from their value to their label did not work well because the values may be non-hashable or hash inconsistently. Instead this PR switches to using the value's identity to do the reverse mapping which should work reliably.