Add MLEngine interface #33
Conversation
| * | ||
| */ | ||
|
|
||
| package org.opensearch.ml.engine; |
There was a problem hiding this comment.
Curiously, should the ML engine under the ml-algorithms project? Probably it's better to put the algorithms under the ML engine?
There was a problem hiding this comment.
Yeah, I got your point. Actually This MLEngine class is a public interface of all ml algorithms, so I put it in ml-algorithms project.
ml-algorithms/src/main/java/org/opensearch/ml/engine/MLEngine.java
Outdated
Show resolved
Hide resolved
ml-algorithms/src/main/java/org/opensearch/ml/engine/MLEngine.java
Outdated
Show resolved
Hide resolved
ml-algorithms/src/main/java/org/opensearch/ml/engine/contants/MLAlgoNames.java
Show resolved
Hide resolved
ml-algorithms/src/main/java/org/opensearch/ml/engine/regression/LinearRegression.java
Show resolved
Hide resolved
ml-algorithms/src/test/java/org/opensearch/ml/engine/helper/KMeansHelper.java
Show resolved
Hide resolved
ml-algorithms/src/test/java/org/opensearch/ml/engine/helper/LinearRegressionHelper.java
Show resolved
Hide resolved
| if (parameters == null) { | ||
| parameters = new ArrayList<>(); | ||
| } | ||
| switch (algoName.toLowerCase()) { |
There was a problem hiding this comment.
Probably we need to validate the parameters here? for example, if the algoName is null, then we will get a NullPointerException.
There was a problem hiding this comment.
I am not so sure about it. I think throwing a NullPointerException is OK, isn't it?
ml-algorithms/src/main/java/org/opensearch/ml/engine/clustering/KMeans.java
Show resolved
Hide resolved
| Model model = trainKMeansModel(); | ||
| Assert.assertEquals("KMeans", model.getName()); | ||
| Assert.assertEquals(1, model.getVersion()); | ||
| Assert.assertNotNull(model.getContent()); |
There was a problem hiding this comment.
Could we verify the model is the expected one instead of only verify it's not null?
ml-algorithms/src/test/java/org/opensearch/ml/engine/MLEngineTest.java
Outdated
Show resolved
Hide resolved
| switch (algoName.toLowerCase()) { | ||
| case MLAlgoNames.KMEANS: | ||
| KMeans kMeans = new KMeans(parameters); | ||
| return kMeans.train(dataFrame); |
There was a problem hiding this comment.
how to handle the case when the train isn't supported by some algorithms?
There was a problem hiding this comment.
enter default branch and throw exception
* add MLEngine interface
Description
Add MLEngine interface
Issues Resolved
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.