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

Only create MatrixStatsResults on final reduction #38130

Merged

Conversation

Projects
None yet
7 participants
@polyfractal
Copy link
Member

commented Jan 31, 2019

MatrixStatsResults is the "final" result object, and runs an additional computation in it's ctor to calculate covariance, etc. This means it should only run on the final reduction instead of on every reduce. But today each round of reductions will create a new MatrixStatsResults object and execute compute(), which I think is where the error in #37587 is coming from.

I'm not 100% certain I understand how MatrixStats works, but based on the test failure and the suggested fix in this PR, I think that's what's going on.

As an aside, we should probably do some refactoring on MatrixStats, so that it doesn't need to pass around a null MatrixStatsResults to distinguish between "intermediate" and "final" status.

Closes #37587

Only create final MatrixStatsResults on final reduction
MatrixStatsResults is the "final" result object, and runs an additional
computation in it's ctor to calculate covariance, etc.  This means
it should only run on the final reduction instead of on every reduce.
@elasticmachine

This comment has been minimized.

Copy link

commented Jan 31, 2019

@polyfractal

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/default-distro

@polyfractal

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

@elasticmachine run elasticsearch-ci/2

@colings86
Copy link
Member

left a comment

I agree that we should refactor this so it doesn't rely on the null results value to differentiate between intermediate and final results especially as null results is also used to indicate an empty result too

@nknize

nknize approved these changes Feb 4, 2019

Copy link
Member

left a comment

👍 LGTM

@polyfractal

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

@elasticmachine run elasticsearch-ci/default-distro

@jasontedor jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

@danielmitterdorfer danielmitterdorfer added v7.2.0 and removed v6.7.0 labels Feb 7, 2019

@polyfractal polyfractal added the v7.0.0 label Feb 7, 2019

@polyfractal

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

@elasticmachine run elasticsearch-ci/2

@polyfractal polyfractal removed the v7.0.0 label Feb 15, 2019

@polyfractal polyfractal merged commit ba98f67 into elastic:master Feb 20, 2019

8 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@polyfractal polyfractal added the v7.0.0 label Feb 21, 2019

@polyfractal polyfractal added the v6.7.0 label Feb 21, 2019

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Feb 22, 2019

Only create final MatrixStatsResults on final reduction (elastic#38130)
MatrixStatsResults is the "final" result object, and runs an additional
computation in it's ctor to calculate covariance, etc.  This means
it should only run on the final reduction instead of on every reduce.

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Feb 22, 2019

Only create final MatrixStatsResults on final reduction (elastic#38130)
MatrixStatsResults is the "final" result object, and runs an additional
computation in it's ctor to calculate covariance, etc.  This means
it should only run on the final reduction instead of on every reduce.

@michaelbaamonde michaelbaamonde added v7.0.0-rc1 and removed v7.0.0 labels Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.