[EEG] EEG Acquisition form

[regisoc]

Brief summary of changes

EEG Acquisition form instrument

Screenshots:

Click me

[christinerogers]

Thanks @regisoc for contributing this for the 25 release, I'll look at the instrument in action too (and it will help also once you post screenshots in the description) but for now some initial comments:

• the instrument should be called "EEG acquisition form" (it's missing the 3rd word)
• Per dave's point that sample instruments don't get patches - see other sample LORIS instruments here
• Were there reasons you went with PHP rather than LINST? is there complex branching logic, e.g.

[regisoc]

Here are the screenshots. Did you test the form last Tuesday on my machine?

Edit: screenshots put in the description.

[regisoc]

@christinerogers @driusan format changed to linst + rules files + json data storage

[regisoc]

@christinerogers Also, do you want this EEG Acquisition form to be permanently in raisinbread?

[ridz1208]

see comments

• there should not be new lines at the end on these files they can break the parser
• there should probably be RB data that goes with it but that can come in a separate PR (at least the insert statemnts)

[ridz1208]

@GeorgeMurad can you make sure to test the data dict builder with it as well as testing the instrument itself

[christinerogers]

Hi @regisoc
Great first draft, Once you get a chance to make these changes (shouldn't take more than an hour) and let me know in slack when it's ready for review.
When you commit, please take it out of Draft

Tip from the Loris meeting discussion: how to check for behavio(u)r spelling consensus, and put your findings back in the agenda when done :

• check the likeliest front-end modules and get a sense from reading them: - do they use vior or viour ? this is the one we want to be consistent with.
• For the whole codebase You can find out the frequency count for behavio(u)ral in the corpus by grepping and counting recursively on your whole local codebase something like grep -ro "ehavio." $yourLorisDirectory | sort | uniq -c 1

[regisoc]

Excluding node_modules and project folder to target only LORIS Core code, it seems the big winner is behaviour.

# behavior
/var/www/Loris$ grep -ro --exclude-dir=project --exclude-dir=node_modules "ehavior" . | sort | uniq -c | awk '{s+=$1} END {print s}'
3598
# behaviour
/var/www/Loris$ grep -ro --exclude-dir=project --exclude-dir=node_modules "ehaviour" . | sort | uniq -c | awk '{s+=$1} END {print s}'
5067

[christinerogers]

Hi @regisoc - thanks for incorporating the previous changes requested.
I've run the latest version by an external eeg expert. My comments and suggestions below reflect their suggestions which will make this form usable across many more eeg studies.
These include:

• generalizing the device language so that cap and montage studies (not just net) can use this form
• in general, let's add a "not known" to yes/no questions.
• I added one question: was a head rest used?
• for non-pediatric studies, tweaked some language
• also changed the subject mood field name (it was eeg_mood)
  I can test this on your VM tomorrow, just let me know when updated and ready -- thanks

[regisoc]

@christinerogers made the changes! Ready for review.I will send you the steps for testing on my vm.

[christinerogers]

Hi @regisoc I started re-reviewing on your vm --

the dropdown selects all need a null value -- i just noticed now they're not formed in the typical way.
they should all have a first NULL option like this:
https://github.com/aces/Loris/blob/main/raisinbread/instruments/bmi.linst#L7
See example BMI form on Demo.loris.ca
without this null first option, In your form all the selects automatic load the first option by default -- see example screenshot:

This should be a quick fix. Let me know when it's ready and I'll re-review by Monday lunch.

[regisoc]

Done you can refresh the page you are on to see the changes

[christinerogers]

I'm just now realizing you generated this linst manually instead of with the Instrument builder.
Please load this in the instrument builder module (on your VM or demo), then save it, and compare the output -- you should see among other Linst format points :

• Null options are added to selects (per my previous review)
• not_answered option for each dropdown
• time type is i think deprecated; that's why it doesn't show in your VM front-end

Please also :

• change do not know to not_known "not known" for each dropdown
• remove : at the end of a field label (because it looks random)
• see other gallicismes or inconsistencies i flagged in comments below

[christinerogers]

I'm just now realizing you generated this linst manually instead of with the Instrument builder.

Please load this in the instrument builder module (on your VM or demo), then save it, and compare the output -- you should see among other Linst format points :

• Null options are added to selects (per my previous review)

• not_answered option for each dropdown

• time type is i think deprecated; that's why it doesn't show in your VM front-end

Please also :

• change do not know to not_known "not known" for each dropdown

• remove : at the end of a field label (because it looks random)

• see other gallicismes or inconsistencies i flagged in comments below

@regisoc could you confirm a few things :

• does your latest push address my last comments ?
• does the instrument work? Does it successfully save data in All fields in the back-end table on your VM?
• when there's a moment Monday, thanks for updating the screenshots in PR
  Let me know when it's ready for final review, thanks --

[regisoc]

@christinerogers to confirm the few things:

• yes, my latest push address your comments. I added one more because I reviewed it and saw one participant->subject rule missing.
• yes, the instrument works on my machine, I can see data are written in the database. It should be good to test on someone else machine just to be sure.
• it is ready for final review. I will update the screenshots right away.

[christinerogers]

I see a few minor issues (#8491) which shouldn't hold up the 25 merge / release

@regisoc please just add one missing rule, and i'll approve. Hopefully this last change will take 90 seconds maximum:

• for the question that follows Subject 60m away : if no, please explain needs a rule (only required if the previous response is "no")

Thanks -- very nearly there

[christinerogers]

@GeorgeMurad could you please run this through the data dict builder as @ridz1208 requested

@ridz1208 your review (below) seems to be addressed -- I'll dismiss, Want to re-review ?

• there should not be new lines at the end on these files they can break the parser. -> see commit #d7d44c0
• there should probably be RB data that goes with it but that can come in a separate PR (at least the insert statemnts)

-> RB data can come for next release, not necessary now -- per last Loris meeting

otherwise this is one small rule change away from merge-ready imo, @driusan

were addressed (see my last comment).

[regisoc]

I see a few minor issues (#8491) which shouldn't hold up the 25 merge / release

@regisoc please just add one missing rule, and i'll approve. Hopefully this last change will take 90 seconds maximum:

* [x]  for the question that follows `Subject 60m away` :  `if no, please explain` needs a rule (only required if the previous response is "no")

Thanks -- very nearly there

@christinerogers It was already there, I just did not apply that to the right folder on my machine so you were not able to see it. My bad. (see line 9 of the rule file) subject_away_60cm_from_monitor_reason{-}Required if subject cannot be 60cm away from montior{-}subject_away_60cm_from_monitor{@}=={@}no

[regisoc]

@driusan ready for a final review