Refactor `expDesign` #84

marcobarilari · 2021-07-03T10:26:33Z

This refers to the function in use for MT+ localizer (nb: MT/MST has its own design function for the moment)

TO DO:

Refactoring

make it "stand alone" make it possible to run the design without running the experiment #69
make vars name transparent expDesign and annexed subfun is a bit difficult to read, consider make it more transparent #67
make fixation target set a separate function
modularize the function for "task" (considering to make them part of CPP_PTB)

Features improvements

~~make it flexible in case someone wants only one condition (either static or motion or more than two conditions)~~ moved to Make design flexible in case someone wants only one condition (either static or motion or more than two conditions) #88
add a task for different speed targets (either faster or slower), might be better to develop this in another or to easy the weight of this one. The ground is set though. Consider as the output of the function a matrix with 1 (is target) and 0 (no target) instead of the actual dot speed moved to add a task for different speed targets (either faster or slower) #89
~~improve and avoid error('number of repetitions must be a multiple of max number of targets');~~ moved to improve and avoid error('number of repetitions must be a multiple of max number of targets'); in expDesign #90
make the target tasks optional (eg if I have two targets like fixation cross and speed, I set cfg.target.type = {'fixation_cross', 'speed'};)
reconsider pseudorand. rule 3 in setFixationTargets: no more than ``nbRepetitions - 1`` target in the same event order. Maybe better a rule of eg no less than 1 per each event position in the block (increase an equal distribution of the targets in the all event's position in the blocks across the run)

Annex

improve doc in README
Improve doc in the function
unit test

for more information, see https://pre-commit.ci

codecov · 2021-07-03T10:31:19Z

Codecov Report

Merging #84 (50e6683) into dev (6854071) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 50e6683 differs from pull request most recent head 1a9573c. Consider uploading reports for the commit 1a9573c to get more accurate results

@@          Coverage Diff          @@
##             dev     #84   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         12      14    +2     
  Lines        220     224    +4     
=====================================
- Misses       220     224    +4

Flag	Coverage Δ
unittests	`0.00% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
subfun/expDesign.m	`0.00% <0.00%> (ø)`
subfun/expDesignMtMst.m	`0.00% <ø> (ø)`
subfun/getDesignInput.m	`0.00% <0.00%> (ø)`
subfun/setBlocksConditions.m	`0.00% <0.00%> (ø)`
subfun/setDirections.m	`0.00% <0.00%> (ø)`
subfun/setFixationTargets.m	`0.00% <0.00%> (ø)`
subfun/setSpeedTargets.m	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f379d8...1a9573c. Read the comment docs.

for more information, see https://pre-commit.ci

…te a matrix of zeros (ergo no change of fixation to show)

for more information, see https://pre-commit.ci

…arco_streamline-expdesign"

for more information, see https://pre-commit.ci

Remi-Gau · 2021-07-30T14:54:09Z

added some tests.

do you want to also refactor the MT MST one ? It should be fairly straight forward now, no? (famous last words)

subfun/design/setSpeedTargets.m

marcobarilari · 2021-08-03T09:56:25Z

tests/test_setSpeedTargets.m

+    assertEqual(speeds, ones(20, 12) * 28);
+
+    % try when the target are just for the fixation cross
+    cfg.target.type = {'fixation_cross'};


why fixation targets here?

if there are only fixation cross as targets then setSpeedTargets must return only zeros.

So it is a second test.

Will break it down into 2 to avoid confusion

Ok here I have "updated" the test meaning that now, being WIP, the two tests are checking for the same results (a matrix full of ones).

Added a reminder in #89

marcobarilari · 2021-08-03T12:54:53Z

Hey sorry for being late on the reply here.

do you want to also refactor the MT MST one ? It should be fairly straight forward now, no? (famous last words)

I am tempted to do that in a new PR cause I suspect that a separate design for MT_MST might be useless now and with few 'if/switch' statements we can do the trick.

Also, I see that in some functions the copyright tag has been moved below the function [out] = myFunc(in) call. The style used 'everywhere' is to put it in the very first line, isn't it?

I am a bit confused by the test for the speed task because I see code related to the fixation task there. So my idea is to use setSpeedTargets to implement a task where the subj has to detect the eg 'faster dots trial'. The output matrix is full of values that are multiplicators of cfg.dot.speed = 15; so that if the matrix reports a 1, then it has the set speed (no change in speed), if eg 2 for a given trial then that speed will be doubled. I know it is hard to get from the code as it is right now but it is because it is WIP and to implement when someone will need that.
The idea is also the possibility to have >1 task in the same experiment if needed, as fixation target && dot faster speed target

Remi-Gau · 2021-08-03T13:16:39Z

Also, I see that in some functions the copyright tag has been moved below the function [out] = myFunc(in) call. The style used 'everywhere' is to put it in the very first line, isn't it?

Until recently miss_hit only allowed the top line, but that would mess up using the "help" command on Octave and very few matlab codebases put the copyright there.

So this has changed and we can now use the more conventional "copyright at the end of the help section".

No need to change all functions now, but if you run into a function where it is still in the top line, put it at the end of the help section.

Remi-Gau · 2021-08-03T13:16:57Z

I am tempted to do that in a new PR cause I suspect that a separate design for MT_MST might be useless now and with few 'if/switch' statements we can do the trick.

Agreed

Remi-Gau · 2021-08-03T13:18:34Z

I am a bit confused by the test for the speed task because I see code related to the fixation task there. So my idea is to use setSpeedTargets to implement a task where the subj has to detect the eg 'faster dots trial'. The output matrix is full of values that are multiplicators of cfg.dot.speed = 15; so that if the matrix reports a 1, then it has the set speed (no change in speed), if eg 2 for a given trial then that speed will be doubled. I know it is hard to get from the code as it is right now but it is because it is WIP and to implement when someone will need that.

Ah OK I get it now: I was not sure what was expected so I sort of made a wild guess: then the code and test need to be updated.

Remi-Gau · 2021-08-03T14:35:52Z

subfun/design/setSpeedTargets.m

+    if ismember('speed', cfg.target.type)
+        [~, nbEventsPerBlock, ~, nbBlocks] = getDesignInput(cfg);
+        speeds = ones(nbBlocks, nbEventsPerBlock) * cfg.dot.speedPixPerFrame;
+
+        % Outputs an "empty" matrix in case no speed task is required
+    else
+        [~, nbEventsPerBlock, ~, nbBlocks] = getDesignInput(cfg);
+        speeds = ones(nbBlocks, nbEventsPerBlock) * cfg.dot.speedPixPerFrame;
+
+    end


I know what confused me on this function.

At the moment both sides of the if block do exactly the same thing, so I thought either the If block is not necessary or the else part must be doing something different from the rest.

Do you see what I mean ?

Yes you are totally right, kind of impossible to guess. My bad.

Let's say that now the function has the structure for future developments. Let me add some "warnings" here and there to make sure it is clear

just a couple of words below the [WIP] will do. :-)

marcobarilari · 2021-08-03T16:28:44Z

subfun/design/expDesign.m

+
+    % Set to 1 for a visualtion of the trials design order
+    if nargin < 2 || isempty(displayFigs)
+        displayFigs = 0;


Here, I think it is very important to keep mock input in order to run a trial of the design outside the experiment. Important to understand what it is doing and in case of further development to see if the tasks and trials are arranged as needed. See also #69

no worries: I basically took all the mocked content and created the getTestConfig function with it.

We can just rename it ("get mock input"or something ) and move it into the design folder and make a call to that function in the expDesign function.

Makes sense?

Yes it makes sense, will also improve the doc related to this. Thanks

for more information, see https://pre-commit.ci

marcobarilari · 2021-08-04T16:03:37Z

I moved the left "to do" to new issues and brought back the mock configuration inside expDesign. Apologies if I push for this but it is a time saver for me and also might help future users to understand how it works and what are the inputs there (hope the new comments are clear).

Also added more info about the WIP situation of setSpeedTargets both in the readme and inside the function. Let me know if the test works after this or it needs a fix.

… marco_streamline-expdesign

marcobarilari · 2021-09-07T14:32:17Z

Merging this one

Fixing the big pr #84

marcobarilari added 3 commits July 1, 2021 09:29

rename variables to their original name

c1d2f2c

keep renaming variable to their original name

044a8b3

add more info for dummy trial

97cb927

marcobarilari self-assigned this Jul 3, 2021

marcobarilari added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed priority 1 High priority labels Jul 3, 2021

[pre-commit.ci] auto fixes from pre-commit.com hooks

fc49b2d

for more information, see https://pre-commit.ci

This was linked to issues Jul 3, 2021

expDesign and annexed subfun is a bit difficult to read, consider make it more transparent #67

Closed

make it possible to run the design without running the experiment #69

Closed

marcobarilari and others added 18 commits July 3, 2021 22:51

take out code for computing design matrices out in separate functions

7a053a2

rename some functions and change the output

f1bfb09

change the name to make it in line with all the other function's names

b5dc551

[pre-commit.ci] auto fixes from pre-commit.com hooks

50e6683

for more information, see https://pre-commit.ci

make fixation targets optional, if not required the design will compu…

900f6b2

…te a matrix of zeros (ergo no change of fixation to show)

debug optional target

d9f5cd3

[pre-commit.ci] auto fixes from pre-commit.com hooks

07a4507

for more information, see https://pre-commit.ci

add some comments to fixation target function

3359f0c

[pre-commit.ci] auto fixes from pre-commit.com hooks

b47095c

for more information, see https://pre-commit.ci

change rule 3 in set fixation targets

2cac5b7

[pre-commit.ci] auto fixes from pre-commit.com hooks

f2499e9

for more information, see https://pre-commit.ci

Auto stash before merge of "marco_streamline-expdesign" and "origin/m…

33a7bf3

…arco_streamline-expdesign"

[pre-commit.ci] auto fixes from pre-commit.com hooks

4d62fb4

for more information, see https://pre-commit.ci

add more comments to setFixationTargets

df46781

add more comments about setFixationTargets

32672b0

[pre-commit.ci] auto fixes from pre-commit.com hooks

e223d11

for more information, see https://pre-commit.ci

prepare setSpeedTargets for further implementation and fix typos

ebf518a

rename vars with transparent name

36c7136

Remi-Gau added 6 commits July 30, 2021 16:11

add test or get design input

ee64bfb

add test get base direction vector

0bd0bf7

add test for set fixation targets

f550ed7

add test for setDrirections

2743ad3

update tests

90252af

lint

ee0796e

Remi-Gau force-pushed the marco_streamline-expdesign branch from 4d77732 to ee0796e Compare July 30, 2021 14:52

marcobarilari commented Aug 3, 2021

View reviewed changes

subfun/design/setSpeedTargets.m Outdated Show resolved Hide resolved

marcobarilari commented Aug 3, 2021

View reviewed changes

Update subfun/design/setSpeedTargets.m

4eb8cc0

Remi-Gau reviewed Aug 3, 2021

View reviewed changes

marcobarilari commented Aug 3, 2021

View reviewed changes

marcobarilari and others added 4 commits August 4, 2021 17:21

add mock config to run expDesign as stand alone

372b426

add WIP warning about the 'speed' task

41a98f4

add more comments to setSpeedTargets

416b588

[pre-commit.ci] auto fixes from pre-commit.com hooks

b0c837e

for more information, see https://pre-commit.ci

marcobarilari requested a review from Remi-Gau August 4, 2021 15:59

marcobarilari added 3 commits August 4, 2021 18:06

fix lint

37a3417

Merge remote-tracking branch 'origin/marco_streamline-expdesign' into…

786434f

… marco_streamline-expdesign

update test checking same results in all the cases , tested func is WIP

1a9573c

marcobarilari merged commit 38710ba into cpp-lln-lab:dev Sep 7, 2021

marcobarilari deleted the marco_streamline-expdesign branch September 7, 2021 14:32

marcobarilari added a commit that referenced this pull request Sep 7, 2021

Merge pull request #92 from marcobarilari/marco_streamline-expdesign

2f68c31

Fixing the big pr #84

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `expDesign` #84

Refactor `expDesign` #84

marcobarilari commented Jul 3, 2021 •

edited

Loading

codecov bot commented Jul 3, 2021 •

edited

Loading

Remi-Gau commented Jul 30, 2021

marcobarilari Aug 3, 2021

Remi-Gau Aug 3, 2021

marcobarilari Sep 7, 2021

marcobarilari commented Aug 3, 2021

Remi-Gau commented Aug 3, 2021

Remi-Gau commented Aug 3, 2021

Remi-Gau commented Aug 3, 2021

Remi-Gau Aug 3, 2021

marcobarilari Aug 3, 2021

Remi-Gau Aug 3, 2021

marcobarilari Aug 3, 2021 •

edited

Loading

Remi-Gau Aug 3, 2021

marcobarilari Aug 4, 2021 •

edited

Loading

marcobarilari commented Aug 4, 2021 •

edited

Loading

marcobarilari commented Sep 7, 2021

Refactor expDesign #84

Refactor expDesign #84

Conversation

marcobarilari commented Jul 3, 2021 • edited Loading

Refactoring

Features improvements

Annex

codecov bot commented Jul 3, 2021 • edited Loading

Codecov Report

Remi-Gau commented Jul 30, 2021

marcobarilari Aug 3, 2021

Choose a reason for hiding this comment

Remi-Gau Aug 3, 2021

Choose a reason for hiding this comment

marcobarilari Sep 7, 2021

Choose a reason for hiding this comment

marcobarilari commented Aug 3, 2021

Remi-Gau commented Aug 3, 2021

Remi-Gau commented Aug 3, 2021

Remi-Gau commented Aug 3, 2021

Remi-Gau Aug 3, 2021

Choose a reason for hiding this comment

marcobarilari Aug 3, 2021

Choose a reason for hiding this comment

Remi-Gau Aug 3, 2021

Choose a reason for hiding this comment

marcobarilari Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Remi-Gau Aug 3, 2021

Choose a reason for hiding this comment

marcobarilari Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

marcobarilari commented Aug 4, 2021 • edited Loading

marcobarilari commented Sep 7, 2021

Refactor `expDesign` #84

Refactor `expDesign` #84

marcobarilari commented Jul 3, 2021 •

edited

Loading

codecov bot commented Jul 3, 2021 •

edited

Loading

marcobarilari Aug 3, 2021 •

edited

Loading

marcobarilari Aug 4, 2021 •

edited

Loading

marcobarilari commented Aug 4, 2021 •

edited

Loading