-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
added linear regression #4
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look at my comments
pom.xml
Outdated
@@ -103,6 +103,21 @@ | |||
<scope>test</scope> | |||
</dependency> | |||
|
|||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not do this but use embedded testing for the library, it's faster than going through server + driver
src/main/java/regression/LR.java
Outdated
} | ||
|
||
@Procedure(value = "regression.linear.storeModel", mode = Mode.WRITE) | ||
public Stream<ModelResult> storeModel(@Name("model") String model) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
save
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's easier to just have a function instead that turns the model into a byte[] so the user can decide themselves what to do with it? i.e. where to store it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree. This was my original idea, but I ran into issues because I was trying to return byte[] as part of a stream (this is silly because I should have just written a user function). I will fix this, and also convert predict into a function that returns a single value rather than a stream.
src/main/java/regression/LR.java
Outdated
} | ||
|
||
@Procedure(value = "regression.linear.createFromStorage", mode = Mode.READ) | ||
public Stream<ModelResult> createFromStorage(@Name("model") String model) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just consume byte[] and turn into Model
} | ||
|
||
//Serializes the object into a byte array for storage | ||
public static byte[] convertToBytes(Object object) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future proof it would probably be better at some point to serialize using some explicit data from the model.
For now it's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I agree, it's just difficult because you cannot create a new SimpleRegression object using the information it stores (because is doesn't actually store individual data points). Maybe I should consider using a different library?
src/main/java/regression/LR.java
Outdated
} | ||
|
||
@Procedure(value = "regression.linear.removeModel", mode = Mode.READ) | ||
public Stream<ModelResult> removeModel(@Name("model") String model) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
src/main/java/regression/LR.java
Outdated
@Procedure(value = "regression.linear.create", mode = Mode.READ) | ||
public Stream<ModelResult> create(@Name("model") String model) { | ||
return Stream.of((new LRModel(model)).asResult()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an info/details function or procedure that just returns data on the model by name.
try{ byte[] serializedR = LR.convertToBytes(R); | ||
Map<String, Object> parameters = new HashMap<>(); | ||
parameters.put("name", name); | ||
ResourceIterator<Entity> n = db.execute("MERGE (n:LRModel {name:$name}) RETURN n", parameters).columnAs("n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to embedded API or just return byte[]
src/test/java/regression/LRTest.java
Outdated
assertThat(actualPrediction, equalTo(expectedPrediction)); | ||
} | ||
|
||
session.run("CALL regression.linear.storeModel('work and progress')"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asserts missing?
src/test/java/regression/LRTest.java
Outdated
"r.time) YIELD prediction SET r.predictedProgress = prediction"); | ||
|
||
//replicate the creation and updates of the model | ||
R.removeData(1.0, 1.345); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there some in-between asserts missing?
src/test/java/regression/LRTest.java
Outdated
|
||
assertThat( actualPrediction, equalTo( expectedPrediction ) ); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove model at in tear-down
The functionality of adding/removing data with SimpleRegression is a bit different than your implementation in ML.java (ex. no need for a separate "train" step) so I created my own package "regression" for these procedures. I also had to add some dependencies to pom.xml. Tests are located in LRTest.java.