dicarlolab / mturkutils

High-throughput web-based human psychophysics
0 stars 4 forks source link

Refactoring common experimental code into dltk #13

Closed hahong closed 10 years ago

hahong commented 10 years ago

@yamins81 @ardila The aim of this branch is to consolidate as much as possible common code in the script section of experimental html files and move it into dltk.js library.

Major improvements are as follows.

The best example to use dltk.Experiment and dltk.RSVPModule is "experiments/objectome_cars_subord/web/cars_subord.html". It should be fairly reasonable to read and understand. Although not recommended, it is still possible to semi-manually construct an RSVP experiment without using dltk.RSVPModule. For that, see "experiments/objectome_cars_subord/web/cars_subord_Experiment.html" (note: but it uses a slightly older version of dltk.Experiment which is not compatible with the current version).

Some important changes:

yamins81 commented 10 years ago

@hahong thanks for this, I'll have a look at it later today.

hahong commented 10 years ago

@yamins81 @ardila On top of the above dev note, there have been two additional changes, which I think makes experimental htmls more clean and coherent:

See "/mturkutils/experiments/objectome_cars_subord/web/cars_subord.html" for details.

ardila commented 10 years ago

This looks good to me, thanks Ha! Once it's merged, I will start on the 8-way

yamins81 commented 10 years ago

I'll review it this afternoon. On Sep 8, 2014 12:03 PM, "Diego Ardila" notifications@github.com wrote:

This looks good to me, thanks Ha! Once it's merged, I will start on the 8-way

— Reply to this email directly or view it on GitHub https://github.com/dicarlolab/mturkutils/pull/13#issuecomment-54843479.

hahong commented 10 years ago

Few improvements work in progress. Please hold reviewing until I push them.

hahong commented 10 years ago

Good to review if you have time --- and just in case, if you haven't, please read all the comments I've put in github PR for the context!

yamins81 commented 10 years ago

@hahong, I've looked at the code and reviewed it some extent, and read the PR notes. I'm not yet totally comfortable merging. I have basically two types of questions:

1) How flexible is the new framework and in what ways do you intend for it to be extensible?

2) How much has the functionality/API in dltk.prepareResources and dltk.queueTrial changed since the version before the refactoring? It doesn't seem to have changed much, which would suggest that it could be used just like before, if necessary. However, in one of your notes above, you see that the version assumed to be used in cars_subord_Experiment is not current. Can you say what has changed? In particular, will any of my experiments still work? What will I have to change to make them work?

yamins81 commented 10 years ago

Also, what would you think about splitting the dltk library into two subsets -- the timing stuff (e.g. what used to be the "old" dltk) and another module containing the Experiment class? I feel like these are somewhat separate things, and it might be easier to think about if they were in different modules.

Specifically, the "old dltk" stuff really needs to be used by any good experiment. The new Experiment class should be optional, and it probably is the case that we'll want to be able to show people how to extend it.

hahong commented 10 years ago

Thanks, @yamins81

1) How flexible is the new framework and in what ways do you intend for it to be extensible?

First, let's not call this as a new framework (sorry if I was confusing). The so-called old part of dltk (including dltk.prepareResources and dltk.queueTrial) remains essentially the same, and the new-old html codes you wrote will still be compatible with the newest dltk.js. This branch essentially adds two classes (Experiment, RSVPModule) that greatly facilitates and de-duplicates experimental implementations.

That being said, like the previous dltk, I feel that the new additions in this branch is quite flexible. In short, I tried to define various time points that are likely to be relevant to UI and graphics controls, and provide some consistent way to insert callback functions for these. Consistency is achieved by three mechanisms: (1) using a dictionary to instantiate objects of newly added classes --- this enforces users to provide all the necessary informations upfront in a structured form; (2) using the same way of handling callback functions (see _callHookfunctions() and uses of it); (3) using a reasonably consistent naming of methods and properties.

The class Experiment does careful initialization of experiment environment that can be used with any type of experiment paradigms even outside of this lab's frame of view. This includes rejecting bad browsers and running benchmarks and interpreting the results correctly. Because of that, I think the class Experiment should be used in any good experiments like the (so-called) old "dltk". Another role of Experiment is managing various modules in a coherent way. These are the main roles of the class, and I don't intent to extend more than that. For instance, Experiment shouldn't contain any task specific implementations; it should be handled by either modules or user-defined callback functions.

The class RSVPModule is specific to RSVP but can be extended to any n-way sample-and-test RSVP tasks. In has many tappable time points, so even without modifying the current "dltk.js" code I think it is possible to implement many of your latent variable tasks by supplying some callback functions when constructing the module.

Speaking back about the flexibility, I tried not to over-split tappable time points to create an efficient library, but it is possible that different users might need time points that I haven't defined yet. Addition such new time points and handling callback functions for those are fairly simple (define a key for the time point in OPTDCT_DEFAULT or OPTDCTMODULE_DEFAULT and call it with _callHookfunctions() when desired).

Also, for drastically new experimental blocks (although I don't have any good example here), users can define a new module and use it with Experiment.

2) How much has the functionality/API in dltk.prepareResources and dltk.queueTrial changed since the version before the refactoring? It doesn't seem to have changed much, which would suggest that it could be used just like before, if necessary. However, in one of your notes above, you see that the version assumed to be used in cars_subord_Experiment is not current. Can you say what has changed? In particular, will any of my experiments still work? What will I have to change to make them work?

Sorry for the confusion. Again, dltk.prepareResources and dltk.queueTrial essentially remain the same. "cars_subord_Experiment.html" is an example to show how to create an RSVP experiment by only using Experiment not RSVPModule. And again, your experiments will work without any modification.

Also, what would you think about splitting the dltk library into two subsets -- the timing stuff (e.g. what used to be the "old" dltk) and another module containing the Experiment class? I feel like these are somewhat separate things, and it might be easier to think about if they were in different modules.

I don't have strong opinion on this, and this sounds reasonable to me. The only reason I didn't split at first was users have to import the old "dltk.js" first and then the new classes later. But in terms of manageability I guess splitting makes sense. I'll reflect this and push with some additional minor modifications.

Specifically, the "old dltk" stuff really needs to be used by any good experiment. The new Experiment class should be optional, and it probably is the case that we'll want to be able to show people how to extend it.

As I said above, I feel Experiment should also be mandatory, because: (1) it greatly increases the likelihood of the so-called-old-dltk's working correctly, (2) using it out of the box is not difficult at all, and (3) it greatly reduces the volume of duplicates codes in the script section in htmls. I also believe that it is best to maximize the use of RSVPModule as well for the same reasons. After all, I have always felt that javascript codes were not version controlled correctly and it's best to maintain a good and tested code in a reasonable way.

yamins81 commented 10 years ago

I have some questions about the specifics of the code, that may turn into suggestions for ways to change it (or may not). Let's talk about this in person today.

yamins81 commented 10 years ago

OK, so we decided to make the following modification to RSVPModule:

(A) document the parameters of the dictionary containing references to various contexts, for use in pre/post Trial functions

(B) include this dictionary in the arguments to the callback of queueTrial

(C) factor out the trial_spec creationfor queueTrial into its own method.

hahong commented 10 years ago

@yamins81 I've made the proposed improvements we talked today:

hahong commented 10 years ago

Also, now Experiment accepts a list of dictionaries in the spec called modulesToAdd, which specifies the modules to be added in Experiment's init(). I added this, because this defines explicit ordering of modules, which would be good for the consistency with the indices used in the spec's trialSpecs[x].ActiveModules. And at the same time, I feel this way of adding modules should be the standard, not user's calling exp.addModule(). See the "cars_subord.html" for details.