-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat: fit functions for null y1 values #416
Conversation
Codecov Report
@@ Coverage Diff @@
## master #416 +/- ##
=========================================
Coverage ? 78.16%
=========================================
Files ? 91
Lines ? 4301
Branches ? 870
=========================================
Hits ? 3362
Misses ? 926
Partials ? 13
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.
Tested locally and everything looks fine.
I've just two minor concerns:
- the static
Explicit
value is not taken in consideration as part of the data domain - I'd like to add a small change on the
endValue
to allow either a value and the nearest point to avoid having to specify a value that maybe is not in the range
Still need to add support for ordinal scale type |
@@ -2,7 +2,9 @@ module.exports = { | |||
roots: ['<rootDir>/src'], | |||
preset: 'ts-jest', | |||
testEnvironment: 'jest-environment-jsdom-fourteen', | |||
setupFilesAfterEnv: ['<rootDir>/scripts/setup_enzyme.ts'], | |||
setupFilesAfterEnv: ['jest-extended', '<rootDir>/scripts/setup_enzyme.ts', '<rootDir>/scripts/custom_matchers.ts'], | |||
coveragePathIgnorePatterns: ['<rootDir>/src/mocks/', '<rootDir>/node_modules/'], |
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.
@markov00 I don't know why the coverage went down to 78% but it does not appear to be because of my code changes. Maybe this is causing some issues. Any thoughts?
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.
I think the current coverage only cover the modules/files imported on each test. Adding the coveragePathIgnorePatterns
maybe count also the missing modules without a test file importing them
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, just few minor comments to fix before merging
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
🎉 This PR is included in version 14.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [14.1.0](elastic/elastic-charts@v14.0.0...v14.1.0) (2019-11-13) ### Features * fit functions for null y1 values ([opensearch-project#416](elastic/elastic-charts#416)) ([6462cff](elastic/elastic-charts@6462cff)), closes [opensearch-project#450](elastic/elastic-charts#450) [opensearch-project#388](elastic/elastic-charts#388)
Summary
Add fit functions below to fill
null
y1
values (y0
null
values not yet supported)Works for ONLY non-stacked series. Stacked series requires another approach having to account for the fitted value of lower series.
[2, null null, 8]
[2, 2, 2, 8]
[2, 2, 8, 8]
[2, 8, 8, 8]
[2, 5, 5, 8]
[2, 4, 6, 8]
null
values with0
[2, 0, 0, 8]
x
), that should be used instead[2, x, x, 8]
Related to fix #388
Usage
within a series spec definition, define the
fit
type.when using the explicit type you must provide a value like so...
By default, indeterminate endvalues, values not able to be fitted, are set to
null
. To set a specific value rather thannull
, you can pass anendValue
prop like so...Demos
[Non-stacked]
Assuming the following data
Fit.None
[default]
Fit.Carry
Fit.Nearest
Fit.Lookahead
Fit.Average
Fit.Linear
Fit.Zero
Fit.Explicit
with value set to4
Curved Linear
Also Works with
curves
Using
endValue
The
endValue
property allows you to set the endnull
point values to a specific value. This value is only used in the case where the fitting function does not have sufficient information to determine the fitted value. For example, usingFit.Average
requires both the trailing and leading non-null boundaries, thus if the endpoints are both null the value can not be found for either.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.src/index.ts
(and stories only import from../src
except for test data & storybook)