rui-han / jspsych-modularization

0 stars 0 forks source link

fixing jsPsych references #5

Open jkhartshorne opened 4 years ago

jkhartshorne commented 4 years ago

We're trying to avoid plugins needing to reference jsPsych directly. This is being done by changing various things in plugin.info from objects to strings, and then having plugin.trial include jsPsych as an argument.

For instance, compare the original html-keyboard-response to the new one.

However, some plugins use 'jsPsych.gluginAPI.registerPreload' outside of the plugin.trial() function. We don't have a good work-around yet.

@ryandrew14 @jodeleeuw

jodeleeuw commented 4 years ago

Here's my current idea.

This is the preload algorithm right now: https://github.com/jspsych/jsPsych/blob/master/jspsych.js#L2504-L2573. It basically runs through all the trials in the experiment and pulls all the stuff that should be preloaded, relying on the registration to determine which things should be preloaded. We could probably remove the registration and instead use plugin.info as the means to determine what should be preloaded. So this loop could check each plugin's info to see what parameters should be preloaded. That'll be a little less efficient than register the preloads, but I think it could work.

rui-han commented 4 years ago

Can we just move jsPsych.pluginAPI.registerPreload(); under plugin.trial?

I tried the simple reaction task, modified the image-keyboard-response plugin and it worked well: https://github.com/hrcn/jspsych-modularization/tree/simple-reaction-time

jodeleeuw commented 4 years ago

No, unfortunately this won't work. The preload function runs at the start of the experiment, before any calls to plugin.trial occur. It may appear like it works when you move it under plugin.trial, but if you check the network activity tab in your browser debugger you'll see that the image file doesn't load until the first time the trial runs.

ryandrew14 commented 4 years ago

Why don't we want plugins to access jsPsych directly?

ryandrew14 commented 4 years ago

What Josh suggested sounds good to me. Doesn't seem like the performance hit will end up mattering that much since the amount of parameters for a given trial (and subsequently plugin.info) probably won't ever get too big. In fact, if we care a lot about the performance hit there, maybe it would be more similar to the existing API if we add a field to plugin.info called preloads which is a list of maybe objects of the form { parameter: <paramName>, mediaType: <mediaType>, conditionalFunc: <condFn> }. This makes it so we don't have to loop over all the parameters to find the preloads.

I think it's also pretty intuitive to have the preloads be part of plugin.info, which already holds other static information with which to set up the trials of that type, like default values.

rui-han commented 4 years ago

@ryandrew14 Do you have any idea about how to modify jspsych.js to do that?

rui-han commented 4 years ago

@jodeleeuw I struggled for a while but I am still not sure how to pass plugin.info parameters to the autoPreload method... And what do you think of @ryandrew14 's solution?

jodeleeuw commented 4 years ago

Why don't we want plugins to access jsPsych directly?

This creates some hidden folder structure dependencies for modularizing the code. Every plugin would need to import jsPsych, and we would need to define the directory structure ahead of time. If a new user accidentally set things up in a non-standard way this would be a confusing problem to troubleshoot.

Plus it seems cleaner and more portable to avoid having plugins directly import jspsych.

jodeleeuw commented 4 years ago

Yeah, I like the idea of plugin.info.preloads. That's a simple solution to the issue!

jodeleeuw commented 4 years ago

As for how to implement...

I think we need to replace this chunk of code:

    for (var i = 0; i < preloads.length; i++) {
      var type = preloads[i].plugin;
      var param = preloads[i].parameter;
      var media = preloads[i].media_type;
      var func = preloads[i].conditional_function;
      var trials = timeline.trialsOfType(type);
      for (var j = 0; j < trials.length; j++) {

        if (trials[j][param] && typeof trials[j][param] !== 'function') {

          if ( !func  || func(trials[j]) ){
            if (media === 'image') {
              images = images.concat(jsPsych.utils.flatten([trials[j][param]]));
            } else if (media === 'audio') {
              audio = audio.concat(jsPsych.utils.flatten([trials[j][param]]));
            }
            else if (media === 'video') {
              video = video.concat(jsPsych.utils.flatten([trials[j][param]]));
            }
          }
        }
      }
    }

Instead of iterating through the preloads list, we need to iterate through the entire timeline and pull the plugin.info.preloads for each trial. Then we can construct the same kinds of lists (audio, images, video), and run the rest of the preload script as before.

The next question is how to iterate through the entire timeline. I actually don't think I've set up a very good API for that. The timeline object that users submit when calling jsPsych.init() is available through jsPsych.opts.timeline. Maybe that's the best way to get that info for now?

(This does suggest that some rethinking of the timeline API is in order...)

rui-han commented 4 years ago

@jodeleeuw So I added preloads to the plugin.info in image-keyboard-response:

plugin.info = {
    name: 'image-keyboard-response',
    description: '',
    parameters: { ... },
    preloads: {
      plugin: 'image-keyboard-response',
      parameter: 'stimulus',
      media_type: 'image'
    }
  }

In html-keyboard-response, it is preloads: undefined.

Then pass opts.timeline in jsPsych.init through:

jsPsych.pluginAPI.autoPreload(timeline, startExperiment, opts.preload_images, opts.preload_audio, opts.preload_video, opts.show_preload_progress_bar, opts.timeline);

In jsPsych.pluginAPI.autoPreload, I modified the loop and I think it iterates the timeline and pull the preloads information for each trial:

 module.autoPreload = function(timeline, callback, images, audio, video, progress_bar, opts_timeline) {
    // list of items to preload
    images = images || [];
    audio = audio || [];
    video = video || [];

    for (var i = 0; i < opts_timeline.length; i++) {
      if(typeof opts_timeline[i].type !== 'undefined' && typeof opts_timeline[i].type.preloads !== 'undefined') {
        var preloads = opts_timeline[i].type.info.preloads;
        var type = preloads.plugin;
        var param = preloads.parameter;
        var media = preloads.media_type;
        var func = preloads.conditional_function;
        var trials = timeline.trialsOfType(type);
        for (var j = 0; j < trials.length; j++) {
          if (trials[j][param] && typeof trials[j][param] !== 'function') {
            if (!func || func(trials[j])) {
              if (media === 'image') {
                images = images.concat(jsPsych.utils.flatten([trials[j][param]]));
              } else if (media === 'audio') {
                audio = audio.concat(jsPsych.utils.flatten([trials[j][param]]));
              }
              else if (media === 'video') {
                video = video.concat(jsPsych.utils.flatten([trials[j][param]]));
              }
            }
          }
        }
      }
      if(typeof opts_timeline[i].timeline !== 'undefined') {
        for (var m = 0; m < opts_timeline[i].timeline.length; m++) {
          var preloads = opts_timeline[i].timeline[m].type.info.preloads;
          if(typeof preloads !== 'undefined') {
            var type = preloads.plugin;
            var param = preloads.parameter;
            var media = preloads.media_type;
            var func = preloads.conditional_function;
            var trials = timeline.trialsOfType(type);
            for (var n = 0; n < trials.length; n++) {
              if(trials[n][param] && typeof trials[n][param] !== 'function') {
                if (!func || func(trials[n])) {
                  if (media === 'image') {
                    images = images.concat(jsPsych.utils.flatten([trials[n][param]]));
                  } else if (media === 'audio') {
                    audio = audio.concat(jsPsych.utils.flatten([trials[n][param]]));
                  }
                  else if (media === 'video') {
                    video = video.concat(jsPsych.utils.flatten([trials[n][param]]));
                  }
                }
              }
            }
          }
        }
      }
    }
 // ...
 }

The complete code here: https://github.com/hrcn/jspsych-modularization/tree/simple-reaction-time

Is this approach feasible?

jodeleeuw commented 4 years ago

This seems like it is the right idea. Couple suggestions:

  1. The current preloads entry has a redundant plugin name. We can access that through the root plugin.info object. I think preloads should be an array:
preloads: [
  {parameter: 'stimulus', media_type: 'image'}
]

This is just in case there are multiple parameters that need to be preloaded. There's at least one plugin where this is the case.

  1. There's a big chunk of repeated code in the autoPreload function now. Maybe this can be separated out into a helper function or refactored some other way?
rui-han commented 4 years ago

I changed preloads into array and refactored the code using forEach method:

Iterate through the entire timeline, if there's a nested timeline, iterate through it. Otherwise wrap the trial in an array (since it is an array method) and iterate it.

    // Iterate entire timeline
    opts_timeline.forEach(multiple_trials => {
      // If there's a nested timeline, iterate through it. Otherwise wrap the trial in an array and iterate it. 
      (multiple_trials.timeline || [multiple_trials]).forEach(trial => {
        var preloads = trial.type.info.preloads;
        if (typeof preloads !== 'undefined') {
          // Loop through preloads array
          for (var i = 0; i < preloads.length; i++) {
            var type = trial.type.info.name;
            var param = preloads[i].parameter;
            var media = preloads[i].media_type;
            var func = preloads[i].conditional_function;
            var trials = timeline.trialsOfType(type);
            for (var j = 0; j < trials.length; j++) {
              if (trials[j][param] && typeof trials[j][param] !== 'function') {
                if (!func || func(trials[j])) {
                  if (media === 'image') images = images.concat(jsPsych.utils.flatten([trials[j][param]]));
                  else if (media === 'audio') audio = audio.concat(jsPsych.utils.flatten([trials[j][param]]));
                  else if (media === 'video') video = video.concat(jsPsych.utils.flatten([trials[j][param]]));
                }
              }
            }
          }
        }
      })
    });
rui-han commented 4 years ago

How do you think this preload algorithm? @jodeleeuw @jkhartshorne

jodeleeuw commented 4 years ago

If you've tested it and it works then I'm on board! I'd suggest adding some automated tests to the testing suite to ensure that preloading behavior is working as expected for different scenarios.

jkhartshorne commented 4 years ago

We'll probably have to look more into the testing. It's new for both of us.

jodeleeuw commented 4 years ago

So just to get you pointed in the right direction.

The tests are written to run with jest.

You can navigate to the tests directory, run npm install to install jest, and then run npm test to run the test suite.

You can add a test anywhere in the tests folder. If you end the filename with .test.js then jest will automatically ID it as a test to run.

Tests will probably need some overhauls to support the modularization. But you can start with a single new test file for the preloading just to verify. Following any of the files for examples. It's not too different than writing normal jsPsych code with the addition of special functions for expected results.