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

fix(computeStats) #427 Exception thrown when firstQuartile and thirdQuartile are equal #841

Merged
merged 3 commits into from
Oct 6, 2020

Conversation

LethalPants
Copy link
Contributor

@LethalPants LethalPants commented Oct 5, 2020

🐛 Bug Fix

test-result
Test result

@coveralls
Copy link

coveralls commented Oct 5, 2020

Pull Request Test Coverage Report for Build 290721624

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.9%) to 56.697%

Files with Coverage Reduction New Missed Lines %
packages/visx-xychart/src/components/XYChart.tsx 2 92.5%
packages/visx-xychart/src/components/series/BarSeries.tsx 5 58.44%
packages/visx-xychart/src/components/series/LineSeries.tsx 5 41.38%
Totals Coverage Status
Change from base Build 82: 0.9%
Covered Lines: 1250
Relevant Lines: 2128

💛 - Coveralls

@LethalPants LethalPants changed the title Computestats exception fix(computeStats) #427 Exception thrown when firstQuartile and thirdQuartile are equal Oct 5, 2020
Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @LethalPants thanks for the fix AND the test 🚀 ! I had one suggestion but otherwise looks good.

packages/visx-stats/src/util/computeStats.ts Outdated Show resolved Hide resolved
@hshoff hshoff self-requested a review October 6, 2020 14:44
@hshoff
Copy link
Member

hshoff commented Oct 6, 2020

One thing that tripped me up on this is computeStates(data).boxPlot.min and computeStates(data).boxPlot.max are not the min/max value in the dataset but the the low (Q1 - 1.5 * IQR) & high (Q3 + 1.5 * IQR) values which are used for calculating the outliers and are the min/max when showing outliers. But when there are no outliers min/max should be the Math.min(data) and Math.max(data).

For example:

data = [10000, 2400, 10000, 10000]
computeStats(data).boxPlot;
{
  "min": 500,
  "firstQuartile": 6200,
  "median": 10000,
  "thirdQuartile": 10000,
  "max": 15700,
  "outliers": []
}

The expected output should be

{
  "min": 2400,
  "firstQuartile": 6200,
  "median": 10000,
  "thirdQuartile": 10000,
  "max": 10000,
  "outliers": []
}

No need to fix this here with this PR, but wanted to flag it as I stumbled on this while reviewing.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks again @LethalPants, looks good.

I created #851 to track the min/max issue @hshoff noted.

@williaster williaster merged commit 9e52d53 into airbnb:master Oct 6, 2020
@LethalPants LethalPants deleted the computestats-exception branch October 7, 2020 04:26
@williaster williaster added this to the 1.1.0 milestone Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception thrown when firstQuartile and thirdQuartile are equal in @vx/stats/computeStats
4 participants