MDL-61667 analytics: Don't autoenable models with time splitting defined
authorDavid Mudrák <david@moodle.com>
Mon, 25 Mar 2019 11:57:31 +0000 (12:57 +0100)
committerDavid Mudrák <david@moodle.com>
Mon, 1 Apr 2019 12:23:06 +0000 (14:23 +0200)
When the API had originally been designed, it was assumed that the
presence of the time splitting method implies no requirement for the
machine learning backend and that it is a sign of less performance
demanding models. So it seemed to be safe and useful to have such models
automatically enabled. That assumption did not prove to be valid. Most
of the time is spent calculating indicators that depend on log tables.

We realized it would be useful to be able to specify the default time
splitting method without the need to have such models auto-enabled.

analytics/classes/model.php
analytics/tests/privacy_test.php

index 6230b75..a96e6a6 100644 (file)
@@ -360,6 +360,16 @@ class model {
         $modelobj->timemodified = $now;
         $modelobj->usermodified = $USER->id;
 
+        if ($timesplittingid) {
+            if (!\core_analytics\manager::is_valid($timesplittingid, '\core_analytics\local\time_splitting\base')) {
+                throw new \moodle_exception('errorinvalidtimesplitting', 'analytics');
+            }
+            if (substr($timesplittingid, 0, 1) !== '\\') {
+                throw new \moodle_exception('errorinvalidtimesplitting', 'analytics');
+            }
+            $modelobj->timesplitting = $timesplittingid;
+        }
+
         if ($processor &&
                 !manager::is_valid($processor, '\core_analytics\classifier') &&
                 !manager::is_valid($processor, '\core_analytics\regressor')) {
@@ -375,10 +385,6 @@ class model {
 
         $model = new static($modelobj);
 
-        if ($timesplittingid) {
-            $model->enable($timesplittingid);
-        }
-
         if ($model->is_static()) {
             $model->mark_as_trained();
         }
index 4e7e2ae..eb3a9fe 100644 (file)
@@ -86,8 +86,10 @@ class core_analytics_privacy_model_testcase extends \core_privacy\tests\provider
 
         $this->setAdminUser();
 
+        $this->model1->enable();
         $this->model1->train();
         $this->model1->predict();
+        $this->model2->enable();
         $this->model2->train();
         $this->model2->predict();