cortex-lab / Rigbox

A MATLAB toolbox for running behavioral neuroscience experiments and managing data
GNU General Public License v3.0
33 stars 16 forks source link

Is it possible to change the array length of parameters in eui.ParamEditor? #311

Open dbirman opened 3 years ago

dbirman commented 3 years ago

Describe the bug It appears to not be possible to change the length of a parameter in the GUI.

To Reproduce Define a parameter in a signals experiment either as a single value or an array:

p.example1 = 1;
p.example3 = [1, 2, 3]';

Using the mc GUI or eui.SignalsTest try to change the length of either parameter.

Setting p.example1 to [1,2] will cause this error:

Unable to perform assignment because the size of the left side is 1-by-1 and the size of the right side is 2-by-1.

Error in eui.ParamEditor/update (line 231)
           obj.Parameters.Struct.(name)(:,row) = newValue;

Setting p.example3 to [1,2] will cause the same error but with a different assignment size.

This appears to be happening because of the (:,row) code at line 231, which forces the matrix to stay the same shape. Removing that extra code allows parameters to change length, but it's unclear if that might be breaking something else up/down stream from here.

p.s. (:,row) is flipped, that's a column?

k1o0 commented 3 years ago

Hi, for better or worse this is the expected behaviour. The issue is that we must enforce an equal number of trial conditions. Trial conditions are defined as parameters with more than 1 column (following MATLAB's column-major order), but the conditions are presented visually as rows in the ParamEditor table, which is kind of confusing, hence the (:, row) is index referring to the table row.

Because the input fields store everything as text, we have to cast the user input back into the underlying format (e.g. '1,2,3' -> [1 2 3]). For most things this is more convenient for the user because they don't have to add braces and quotes, etc. The fact that all trial conditions must share a dimension, and that there is interpretation ambiguity, makes it difficult to implement size and type changes for example when a user writes '1,2,3' do they want three different trial conditions or to change the size of the array?

For most of the experiments we do the parameters are not expected to change type or size. Although this is possible, particularly in Signals, it makes things more difficult because you have to write in cases to deal with dimension mismatches and such. It's very uncommon that a user wants to change the size of their parameters so the safest thing is to throw an error.

We did partially implement a way around this, which was to allow parameters that are cell arrays to be 'free fields', i,e. they could be mixed type and could change type and shape. Again this is tricky because it's hard to interpret the intended input of the user, so with this feature the values all end up a char arrays (if you type '5' do you want it to cast to numeric or keep as a char?).

I guess we could do two things to improve this:

  1. Expose the ParamEditor prop in the mc object, so that users can do these advanced things in the command prompt:

    MC = mc;  % start mc
    MC.Parameters.Struct.example1 = [1, 2, 3]'  % change parameter from number to array
    MC.ParamEditor.buildUI(MC.Parameters)  % clear and rebuild
  2. Add an option in the context menu to change the parameter type. A little window like the one for 'set values' can contain a text box whose input is run through eval, which will parse the input like command prompt input.

  3. We could completely overhaul the Parameters class so that users define two separate structures, one for trial conditions and one for global parameters (#144). We could add support for an additional types table that defines the supported types for each parameter.

The first one is obviously very easy to implement, the second one shouldn't take to long but I'd need to add tests for mc which will take a while. The third option would be a lot of work and, aside from avoiding the row/column size confusion, would not be used very often by users. What do you think about these solutions?

dbirman commented 3 years ago

Thanks for the detailed explanation. Option 1 seems best, if someone ever implements 3 then the exposed property could be removed.

I might still be misunderstanding, but isn't there also an option here to parse anything that includes brackets as numeric (e.g. [1,2,3]') so that you can then interpret whether it's a row or column vector to tell what the user wants to happen? So [1,2,3] is interpreted as conditional and split into the trial conditions, while [1,2,3]' changes the array size of a global parameter?

k1o0 commented 3 years ago

I might still be misunderstanding, but isn't there also an option here to parse anything that includes brackets as numeric (e.g. [1,2,3]') so that you can then interpret whether it's a row or column vector to tell what the user wants to happen? So [1,2,3] is interpreted as conditional and split into the trial conditions, while [1,2,3]' changes the array size of a global parameter?

It could have been implemented that way. It has its advantages and disadvantages. For example fringe cases where the underlying type is not numerical there would be problems. The advantage of the current way is that the user doesn't have to worry about array syntax or shape.

I'll come up with a solution for the next release.