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

[ML-177][Native Bayes] Fix error when converting Vector to CSRNumericTable #176

Merged
merged 9 commits into from
Feb 23, 2022
Merged

[ML-177][Native Bayes] Fix error when converting Vector to CSRNumericTable #176

merged 9 commits into from
Feb 23, 2022

Conversation

minmingzhu
Copy link
Collaborator

  1. fix naiveBayes bug
  2. add unit test for converting vector to CSRNumericTable

Signed-off-by: minmingzhu minming.zhu@intel.com

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

Does this PR also require the following changes?

  • CI
  • Documentation
  • Example

2. add unit test for converting vector to CSRNumericTable

Signed-off-by: minmingzhu <minming.zhu@intel.com>
@github-actions
Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?
https://github.com/oap-project/oap-mllib/issues

Then could you also rename pull request title and commit log in the following format?

[ML-${ISSUES_ID}] ${detailed message}

See also:

@@ -53,7 +53,8 @@ suiteArray=(
"classification.MLlibNaiveBayesSuite" \
"regression.MLlibLinearRegressionSuite" \
"stat.MLlibCorrelationSuite" \
"stat.MultivariateOnlineSummarizerSuite"
"stat.MultivariateOnlineSummarizerSuite" \
"oneDALSuite"
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you put oneDALSuite in com.intel.oap.mllib namespace to align with oneDAL.scala

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if putting oneDALSuite in com.intel.oap.mllib, we can't use Matrices.fromVectors function to convert Vector to Matrix, because fromVectors function was private[ml]. In my opinion, we can distinguish more fine-grained into ML and MLLIb likes following image.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, thanks!

Signed-off-by: minmingzhu <minming.zhu@intel.com>
@xwu99
Copy link
Collaborator

xwu99 commented Feb 21, 2022

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues? https://github.com/oap-project/oap-mllib/issues

Then could you also rename pull request title and commit log in the following format?

[ML-${ISSUES_ID}] ${detailed message}

See also:

Pls follow this format to file an issue first and rename your PR title. Pls be more specific about what you want to do. same for #176

@minmingzhu minmingzhu changed the title NaiveBayes error [ML-177]NaiveBayes error Feb 21, 2022
@github-actions
Copy link

#177

@xwu99 xwu99 changed the title [ML-177]NaiveBayes error [ML-177][Native Bayes] Fix error when converting Vector to CSRNumericTable Feb 21, 2022
@@ -14,7 +14,7 @@
* limitations under the License.
*/

package com.intel.oap.mllib;
package com.intel.oap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep this inside oap mllib namespace. oap space is for the whole oap product. same below.

@@ -236,7 +236,7 @@ object OneDAL {
matrixLabel
}

private def vectorsToSparseNumericTable(vectors: Array[Vector],
def vectorsToSparseNumericTable(vectors: Array[Vector],
nFeatures: Long): CSRNumericTable = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
nFeatures: Long): CSRNumericTable = {
nFeatures: Long): CSRNumericTable = {

Copy link
Collaborator

Choose a reason for hiding this comment

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

pls correct code indent

Signed-off-by: minmingzhu <minming.zhu@intel.com>
@xwu99
Copy link
Collaborator

xwu99 commented Feb 23, 2022

@minmingzhu thanks for your work, I will fix the rest of this PR and merge.

@xwu99 xwu99 merged commit 3a68664 into oap-project:master Feb 23, 2022
@xwu99 xwu99 linked an issue Feb 24, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Native Bayes] Fix error when converting Vector to CSRNumericTable
2 participants