refactor cfg with sub-structure for screen, keyboard...

[Remi-Gau]

I am actually more and more in favor of not only reorganizing this but also of actually merging cfg and expParameters.

Why?

• We frequently pass them both to a function together as arguments (usually an indication that they "belong together").
• It is becoming increasingly difficult to remember in which structure some information goes (e.g the information if we use an eyetracker in in cfg but the name of the file to save eyetracker information is in expParameter)
• This makes it more difficult when coding too because you have to always look up the function help to know which one of the 2 it is it needs.
• This is starting to create some redundancy that I do not like: I have just had to create a cfg.verbose and an expParameter.verbose because some functions call only one or the other so I needed that info to be in both.

I am also trying to think about future users (most likely our future selves) who will inevitably will encounter those problems.

So unless I hear scream of horrors from someone I will start working on a PR on this: first on CPP_BIDS and then on CPP_PTB.

@marcobarilari @CerenB

[CerenB]

Ok. For discussion sake, would it make sense to have some structures inside the cfg? So when one is looking for some parameters it's super difficult to find them atm. The bad part of structure within structure is the variable name starts to be longer. I'm open to suggestions in organising inside this one gigantic cfg as well.

[Remi-Gau]

Yes I agree.

• let's take that opportunity to rearrange the structure itself with sub-structures
• there is a trade off but between organization and longer names indeed
• let's to no end up with a structure where all the names are NOT transparent like the content of SPM.mat

[Remi-Gau]

To be concrete, let's start with CPP_BIDS.

cfg.testingDevice = 'pc';
cfg.eyeTracker = false;

cfg.subjectGrp = '';
cfg.sessionNb = 1;
cfg.askGrpSess = [true true];

cfg.verbose = 0;

cfg.MRI.ce = [];
cfg.MRI.dir = [];
cfg.MRI.rec = [];
cfg.MRI.echo = [];
cfg.MRI.acq = [];

cfg.bids.MRI.RepetitionTime = [];
cfg.bids.MRI.SliceTiming = '';
cfg.bids.MRI.TaskName = '';
%     cfg.bids.MRI.PhaseEncodingDirection = '';
%     cfg.bids.MRI.EffectiveEchoSpacing = '';
%     cfg.bids.MRI.EchoTime = '';
cfg.bids.MRI.Instructions = '';
cfg.bids.MRI.TaskDescription = '';

cfg.bids.datasetDescription.Name = '';
cfg.bids.datasetDescription.BIDSVersion =  '';
cfg.bids.datasetDescription.License = '';
cfg.bids.datasetDescription.Authors = {''};
cfg.bids.datasetDescription.Acknowledgements = '';
cfg.bids.datasetDescription.HowToAcknowledge = '';
cfg.bids.datasetDescription.Funding = {''};
cfg.bids.datasetDescription.ReferencesAndLinks = {''};
cfg.bids.datasetDescription.DatasetDOI = '';

Some comments and suggestions or questions :

• substructure should be mri and not MRI
• some of the fields in the bids are in PascalCase but that's because this is required for the creation of the BIDS JSON
• the way the bids sub-structure is organised is by JSON file: e.g the content of the datasetDescription.json is stored in bids.datasetDescription the content of the sub-*/func/_bold.json for fMRI is in the bids.MRI sub-structure.
• for eyetrackng and audio and any other extra things I am tempted to have structured that way

cfg.eyeTracker.do = false; %boolean flag to say whether this "device" should be used
cfg.eyeTracker.param1Name = value_1; % list all the parameters related to that device with their values (like we did for the audio part) 
cfg.eyeTracker.param2Name = value_2;

• the fieldcfg.MRI is only used for filenaming: maybe this should be moved into the cfg.fileName``structure that thecreateFilename` function creates

• do we want to group the subject info (see below) together (with then subject and run number)?

  cfg.subjectGrp = '';
  cfg.sessionNb = 1;
  cfg.askGrpSess = [true true];

[Remi-Gau]

And this would be the content of cfg afer running the testmanual_makeRawDataset from CPP_BIDS

cfg.MRI= [1×1 struct]
cfg.askGrpSess= [1 1]
cfg.bids= [1×1 struct]
cfg.datasetDescription= [1×1 struct]
cfg.date= '202007271236'
cfg.fileName= [1×1 struct]
cfg.modality= 'func'
cfg.outputDir= '/home/remi/github/CPP_BIDS/tests/../output'
cfg.pattern= '%03.0f'
cfg.runNb= 1
cfg.runSuffix= '_run-001'
cfg.sessionNb= 1
cfg.subjectGrp= ''
cfg.subjectNb= 1
cfg.subjectOutputDir= '/home/remi/github/CPP_BIDS/tests/../output/source/sub-001/ses-001'
cfg.task= 'testtask'
cfg.verbose= 0
cfg.eyeTracker= 0
cfg.testingDevice= 'mri'
cfg.verbose= 0

further comments

• a lot of that should go into the fileName sub-structure:
  • cfg.runSuffix most probably by having a substructure for all the suffixes (i.e cfg.fileName.suggix.run)
  • cfg.pattern= '%03.0f' % pattern used for 0 padding the subject, session, run number
  • cfg.date= '202007271236' % actual date of the experiment that is appended to the filename
  • cfg.datasetDescription actually contains the fulltpath name of the datasetDescription and should go into fileName
• having a directory sub-structure would make sense for the outputDir, subjectOutputDir, no?

[Remi-Gau]

In practice I have also started putting the extraColumns info to create the tsv directly into cfg because the content of that substructure can be quite lengthy if one wants. See below an example:

    expParameters.extraColumns.x_target_pos = struct( ...
        'length', 1, ...
        'bids', struct( ...
        'LongName', 'x position of the the target', ...
        'Units', 'degrees of visual angles'));

    expParameters.extraColumns.y_target_pos = struct( ...
        'length', 1, ...
        'bids', struct( ...
        'LongName', 'y position of the the target', ...
        'Units', 'degrees of visual angles'));

    expParameters.extraColumns.target_width = struct( ...
        'length', 1, ...
        'bids', struct( ...
        'LongName', 'diameter of the the target', ...
        'Units', 'degrees of visual angles'));

It was then easier to do that and then just transfer it to the logFile structure used for saving.

So I would suggest adding it there as well. Maybe even as expParameters.logFile.extraColumns so that the logFile variable initialisation can simply be.

logFile = expParameters.logFile;

[Remi-Gau]

On the CPP_PTB front

These are the defaults.

>> cfg.initAudio = 1;
>> cfg = setDefaultsPTB(cfg)

cfg = 

  struct with fields:

                  audio: [1×1 struct]
        backgroundColor: [0 0 0]
                  debug: 1
              initAudio: 1
               keyboard: [1×1 struct]
           monitorWidth: 42
         screenDistance: 134
          testingDevice: 'pc'
     testingSmallScreen: 1
    testingTranspScreen: 1
                   text: [1×1 struct]

Comments:

• put all the color inforfmation in cfg.color
• cfg.initAudio becomes cfg.audio.do
• all the debug info goes into a debug sub-structure:

  cfg.debug.do
  cfg.debug.smallScreen
  cfg.debug.transpScreen

• all the info about the screen goes into cfg.screen

  cfg.screen.screenDistance
  cfg.scree.monitorWidth

[CerenB]

Thank you for documenting all.

logFile = expParameters.logFile; You meant cfg.logFile; right?

[Remi-Gau]

Thank you for documenting all.

Well I guess that will help for documentation later. :-)

logFile = expParameters.logFile; You meant cfg.logFile; right?

Yes!! :smile:

[Remi-Gau]

And this should be the standard output of initPTB (added comment directly below)

% Should go into cfg.color
cfg.backgroundColor

% all of those go into cfg.screen
cfg.monitorWidth
cfg.screenDistance
cfg.FOV
cfg.screen 
cfg.win 
cfg.winRect 
cfg.winWidth 
cfg.winHeight 
cfg.center 
cfg.ppd 
cfg.ifi 
cfg.monRefresh 
cfg.vbl % useless indeed

cfg.text.Font 
cfg.text.Size 
cfg.text.Style 

cfg.audio.pahandle
cfg.audio.devIdx
cfg.audio.playbackMode
cfg.audio.requestedLatency
cfg.audio.fs
cfg.audio.channels
cfg.audio.initVolume
cfg.audio.pushSize  
cfg.audio.requestOffsetTime 
cfg.audio.reqsSampleOffset

[Remi-Gau]

OK and given the few experiments I started adapting with this framework there seems to be a couple of sub-structure we could initialize to "nudge" users into putting their info there.

• anything that has to with fixation cross (cross or dot, size, color, location...) goes into cfg.fixation I am planning to create a drawFixation function because I am TIRED of rewriting those 3 lines of code a million times. I mean we already have a draft of a drawFixationCross so it would only make sense to have a fully fledge version and henve a dedicated space for that in the cfg.

Do we want to centralize info into the possible following: cfg.task,cfg.stimulusandcfg.target` ?

[Remi-Gau]

OK I think this is for now.

Will let us digest all of this and go back to it a bit later.

[CerenB]

curious, @Remi-Gau or @marcobarilari would not use expParam at all?

[marcobarilari]

I see no problem in centralizing everything in one struc with sub-structs, I would just not go deeper than 2 levels e.g. struct.sub-struct.var.

@CerenB I guess no if we centralize everything there. Right?

[CerenB]

Below could use some subfield as well, maybe cfg.trigger.key and cfg.trigger.num:

cfg.triggerKey    = 's';        
cfg.numTriggers   = 4; 

[Remi-Gau]

curious, @Remi-Gau or @marcobarilari would not use expParam at all?

@CerenB I would try to avoid it as much as possible.

I see no problem in centralizing everything in one struc with sub-structs, I would just not go deeper than 2 levels e.g. struct.sub-struct.var.

I agree.

Below could use some subfield as well, maybe cfg.trigger.key and cfg.trigger.num:

cfg.triggerKey    = 's';        
cfg.numTriggers   = 4; 

Yes but those will go into the cfg.mri so after that I guess we can try to stick to explicit and transparent field names, but we can maybe do the following so at least things 'group" together (will try to remember to sort fields alphabetically with sortfields when setting up all the defaults).

cfg.mri.triggerNb
cfg.mri.triggerKey

[Remi-Gau]

I see no problem in centralizing everything in one struc with sub-structs, I would just not go deeper than 2 levels e.g. struct.sub-struct.var.

actually... We already have more levels that this. :unamused:

cfg.bids.mri.RepetitionTime

So let's make that no more than 3 levels... (quote here Monty python Spanish inquisition sketch...).

[Remi-Gau]

OK I will close this for now.

I have tried to list of the fields and sud-flelds of cfg in the READMES.

Will now update some of our expriement scripts. A good way to make sure evertyhing works