Correct SVM Use#346
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
andrewdalpino
left a comment
andrewdalpino
left a comment
There was a problem hiding this comment.
Nice @LouisAUTHIE thanks great work
I left a few comment and questions
| $this->model = $this->svm->train($dataset->samples()); | ||
| $data = []; | ||
|
|
||
| foreach ($dataset->samples() as $i => $sample) { |
There was a problem hiding this comment.
Looks like we don't need this $i variable
There was a problem hiding this comment.
You are completely right, this is now corrected !
| $data = []; | ||
|
|
||
| foreach ($dataset->samples() as $i => $sample) { | ||
| $data[] = array_merge([1], $sample); |
There was a problem hiding this comment.
I wonder which is faster ...
array_merge([1], $sample)or
array_unshift(1, $sample)From what I recall, unshift is linear because it needs to reindex the array or something. I think merge is also linear.
There was a problem hiding this comment.
Change is done ;)
| $sampleWithOffset[$key + 1] = $value; | ||
| } | ||
|
|
||
| return $this->model->predict($sampleWithOffset) == 1 ? 0 : 1; |
There was a problem hiding this comment.
I notice we are "inversing" the logic here i.e. 1 is now 0, 0 is now 1. Is that intentional?
There was a problem hiding this comment.
Yes, in fact in the one class mode of libsvm, the "normal" samples are to be labelled with the 1 class. And the anomalies, are to be labelled with -1. That's why !
|
|
||
| foreach ($dataset->samples() as $i => $sample) { | ||
| $data[] = array_merge([1], $sample); | ||
| foreach ($dataset->samples() as $sample) { |
There was a problem hiding this comment.
So did array_unshift() turn out to be faster?
Is it necessary to assign the sample to an intermediate variable or would
foreach ($dataset->samples() as $sample) {
array_unshift($sample, 1);
$data[] = $sample;
}work here as well?
There was a problem hiding this comment.
It is faster yes. And the rest works also, please see the next commit
andrewdalpino
left a comment
andrewdalpino
left a comment
There was a problem hiding this comment.
Looks awesome, thank you @LouisAUTHIE
We'll get this deployed in a bugfix release ASAP
2 participants
Correcting the use of SVM for one class anomaly detection