Skip to content

fix LdaWorkoutEstimatorCore #4927

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

Merged

Conversation

frank-dong-ms-zz
Copy link
Contributor

@frank-dong-ms-zz frank-dong-ms-zz commented Mar 10, 2020

fix LdaWorkoutEstimatorCore test.

resetRandomGenerator needs to be true here as multiple compare will be performed later.
In lda_engine, a queue of samples with size of (num_of_threads - 2) will be created at first, each time a compare is performed the internal status of one sample (random number: rng_) is changed, so if size of queue is smaller the number of compare performed (in local workstation we have 12 cores thus the issue is not reproduced), dirty data will be used again for calculation and cause issue. set resetRandomGenerator to true will reset the random number rng_ every time before lda calculation thus fix the issue.

@frank-dong-ms-zz frank-dong-ms-zz requested a review from a team as a code owner March 10, 2020 07:48
@@ -729,7 +728,7 @@ public void LdaWorkoutEstimatorCore()
builder.AddColumn("F1V", NumberDataViewType.Single, data);
var srcView = builder.GetDataView();

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a comment can be added here that explains the need to reset the random number generator for each received document due to way that ML.NET and LdaNative utilize multiple threads differently, for future readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, thanks


In reply to: 390162926 [](ancestors = 390162926)

Copy link
Contributor

@harishsk harishsk left a comment

Choose a reason for hiding this comment

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

:shipit:

@frank-dong-ms-zz frank-dong-ms-zz merged commit ef8091f into dotnet:master Mar 10, 2020
@frank-dong-ms-zz frank-dong-ms-zz deleted the fix-LdaWorkoutEstimatorCore branch April 7, 2020 04:29
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants