-
Notifications
You must be signed in to change notification settings - Fork 218
feat(nimbus): add results indicator next to metric slug in metric area headers #14499
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
Conversation
mikewilli
left a comment
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.
Seeing this, it really drives home that the answer to "was the overall significance of this metric positive or negative?" is quite a bit more nuanced than it sounds. I wonder if it'd be better to simply annotate them with statuses instead. Something like:
- ❗ Notable metric change
- ❌ Metric missing results
- (no special styling/icon) No error or notable change
What do you think?
experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/results-new-inner.html
Outdated
Show resolved
Hide resolved
|
Oh yeah I think I suggested four possible states
But you're right @mikewilli there's also a case for
I think that should cover all the states. Yeah theoretically we could fold any positive/negative together into a 'notable results' I just think it would be great to see all the directionality in one zoomed out view before you zoom in, which we don't have right now. If we find it's confusing or misleading we can always change it later. |
Yep, I planned to address better ui for metrics that are missing data in a different pr |
|
Oh the double arrows means mixed! That's clever, but actually I'd use up/down together since we're using up and down already for each of positive and negative. |
jaredlockhart
left a comment
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.
Yep this is all looking good! Try playing with set and see if it helps, otherwise this should be good to land 🎉
| from experimenter.outcomes import Outcomes | ||
|
|
||
|
|
||
| class MetricSignificance(StrEnum): |
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.
Perfect 🙏
|
|
||
| significances = {metric_significance, overall_change} | ||
|
|
||
| if significances == { |
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.
Nice 🙏


Because
This commit
Fixes #14460