Skip to content
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

Model overview: use spline instead of line chart #1508

Merged
merged 2 commits into from
Jun 20, 2022

Conversation

romanlutz
Copy link
Contributor

Description

This PR replaces the line chart with a spline chart to make it more pleasant to look at. The only real change is that the chart option is changed from "line" to "spline", the rest are renaming changes.

Old experience:

image

New experience:

image

Checklist

  • I have added screenshots above for all UI changes.
  • I have added e2e tests for all UI changes.
  • Documentation was updated if it was needed.

@romanlutz romanlutz requested a review from xuke444 as a code owner June 20, 2022 14:29
@imatiach-msft
Copy link
Contributor

@romanlutz what do you think about giving them the option of doing both? The spline can be misleading in many cases.

@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2022

Codecov Report

Merging #1508 (075b95a) into main (c01744e) will decrease coverage by 2.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1508      +/-   ##
==========================================
- Coverage   89.30%   87.27%   -2.03%     
==========================================
  Files          38      108      +70     
  Lines        1617     5108    +3491     
==========================================
+ Hits         1444     4458    +3014     
- Misses        173      650     +477     
Flag Coverage Δ
unittests 87.27% <ø> (-2.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
raiwidgets/raiwidgets/error_handling.py 100.00% <0.00%> (ø)
raiwidgets/raiwidgets/dashboard.py 92.68% <0.00%> (ø)
.../databalanceanalysis/aggregate_balance_measures.py 92.85% <0.00%> (ø)
raiwidgets/raiwidgets/explanation_constants.py 100.00% <0.00%> (ø)
...ibleai/_tools/causal/dashboard_schemas/__init__.py 100.00% <0.00%> (ø)
...ai/databalanceanalysis/feature_balance_measures.py 100.00% <0.00%> (ø)
raiwidgets/raiwidgets/error_analysis_dashboard.py 72.41% <0.00%> (ø)
...sponsibleai/responsibleai/managers/base_manager.py 95.23% <0.00%> (ø)
...dgets/raiwidgets/error_analysis_dashboard_input.py 66.21% <0.00%> (ø)
...ponsibleai/responsibleai/rai_insights/constants.py 100.00% <0.00%> (ø)
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c01744e...075b95a. Read the comment docs.

@romanlutz
Copy link
Contributor Author

@romanlutz what do you think about giving them the option of doing both? The spline can be misleading in many cases.

Technically, this should be a histogram. However, we didn't find a good way to make a histogram with multiple cohorts that can still be read in a meaningful way (we tried...). The alternative was the line chart. The lines are actually just there to connect the dots, otherwise you would have a tough time figuring out which dots belong together. I think the general perception was that the pure line chart is terribly ugly and nobody wants to look at it. The splines are much more visually pleasing. However, I agree on them having the potential to mislead. That said, we're not saying that this is the actual probability distribution, but just a spline chart of counts (if you look at the axis and toggle).

Two things are still missing here IMO that will also help:

  • give users control of binning on the x-axis - the more bins there are the better of an idea we get of the distribution (at least to a certain number and assuming we have plenty of samples)
  • make the y-axis the percentage of total cohort count rather than an absolute count in the bin. If we did that all the cohorts would probably look the same since they are mostly distinct in their size (number of samples).

Anyway, we can bring this up with PM to see if there's a need to have the pure line chart, too. Seeing as nobody wanted it to begin with and it was just my stopgap solution until I can make a spline chart, I doubt it. Will keep you posted!

@romanlutz romanlutz enabled auto-merge (squash) June 20, 2022 17:17
@romanlutz romanlutz merged commit a33d2ac into main Jun 20, 2022
@romanlutz romanlutz deleted the romanlutz/spline_chart branch June 20, 2022 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants