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 predict difference #6384

Closed
wants to merge 2 commits into from
Closed

Conversation

ShvetsKS
Copy link
Contributor

related issue: #6350

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Could you please provide a brief explanation for what happened? Also a unittest. Lastly I see that you are adding an extra allocation, does it impact perf?

@trivialfis
Copy link
Member

Ah, I saw your reply on the issue. Will continue there.

@ShvetsKS
Copy link
Contributor Author

Could you please provide a brief explanation for what happened? Also a unittest. Lastly I see that you are adding an extra allocation, does it impact perf?

The reason of difference in output result is changed sequence of floating point operations:
Before a4ce0ea optimizations we accumulated trees responses for each sample in local variable psum with 0 initial value. And than increment out_pred[i].
But in a4ce0ea optimizations we increment out_pred[i] directly (initial value 0.5 usually), so the fp error was a little bit different.

There is already implemented cpp tests for cpu predictor (cpu_predictor->PredictBatch), there is no significant changes. Or you mentioned about ReduceResult?

I checked it on several benchmarks, seems there is no performance affection.

@ShvetsKS
Copy link
Contributor Author

ShvetsKS commented Nov 13, 2020

https://xgboost-ci.net/blue/organizations/jenkins/xgboost-win64/detail/PR-6384/3/pipeline#step-89-log-1987
Not sure that it's related to current changes as locally all tests from tests/python/test_with_sklearn.py are passed.

@trivialfis should we restart Jenkins Win64: Test ?

@trivialfis
Copy link
Member

trivialfis commented Nov 13, 2020

@ShvetsKS Let's pause on this with comment: #6350 (comment) . We @RAMitchell @hcho3 agreed that we should not establish the tradition of fixing floating change across versions, as you stated, that will severely limit our development. We haven't decided how to document or whether do we need to document this.

@trivialfis
Copy link
Member

But to reply your question on the failing CI. Yeah, it happens all the time that error is only reproducible on CI ... Something I have been bumping into quite often. The error in this PR is new to me, so it might be specific to this PR instead of CI glitches. You can see on our issues list for all flaky tests we have found.

@ShvetsKS
Copy link
Contributor Author

@trivialfis seems I should close this PR as #6350 was resolved? :)

@hcho3 hcho3 closed this Nov 13, 2020
@hcho3
Copy link
Collaborator

hcho3 commented Nov 13, 2020

Let us continue our discussion in #6350.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants