Gridsearch nkg

[caiw]

Changes

• Fixes Gridsearch should output .nkg files, not .stc files #67: plain gridsearch saves its results as an .nkg file.
• Reuses existing expression_plot() code to do plotting of p-values.
• Uses the central location for converting p-values to log p-values to ensure the bases match.
• Forces loading of EMEG data from fif files, not npy files, as a workaround to io.mne.load_single_emeg doesn't load channel names #129

Note

• When run on hexel data, the plain gridsearch currently only runs on left-hemisphere data (hexel gridsearch only works on lh data #126). This PR does nothing to change this and in fact expects it — easily updated in future, marked with #TODOs.
• No new tests: Tests for gridsearch #124.
• There is currently a syntax error in the gridsearch function, which is not fixed by this PR (see Syntax error in gridsearch/plain.py #128 )

[neukym]

A handful of comments, but otherwise, looks great 👍

[caiw]

I've moved non-universal default values out of the invoker python script and into the shell script. I think it makes more sense for people to keep personal shell script around for their preferred arguments, rather than hard-code them as defaults in the python gridsearch invoker itself

However this does mean one can't just run python run_gridsearch.py with no arguments any more...

[caiw]

However this does mean one can't just run python run_gridsearch.py with no arguments any more...

I've now created invokers/sh/gridsearch.sh which runs the .py invoker with the arguments. A bit of indirection, but I think it helps keep things clean and flexible.

Sorry for post-request changes @neukym!

[neukym]

It looks good. I have a couple of minor suggestions but I just need to think about them.

Can you confirm this works with inverse_operator present though? For me it takes longer than 10 mins on lws-gpu, and I have to stop it. Can we print out some sort of basic progress indicator - I thought ollie had got it to 3 mins or so?

[neukym]

Also can we automatically create /output/ if it doesn't already exist? (sorry)

[caiw]

@neukym

Can you confirm this works with inverse_operator present though?

I'll verify this. (It doesn't error out, but I haven't run it through yet - it's going now.)

[neukym]

To confirm, this now works very quickly, even for hexel space (3.30 seconds) 👍 Ready to merge, awaiting @ollieparish's confirmation.

[ollieparish]

Happy with this too, happy to merge but still not convinced about requiring certain parser options to be passed when running, especially as (I'm assuming) this will be run on the login nodes running random functions on sensor space, having a baseline and then only changing 1 or 2 things is quite helpful (for me at least)

[neukym]

[...] but still not convinced about requiring certain parser options to be passed when running, especially as (I'm assuming) this will be run on the login nodes running random functions on sensor space, having a baseline and then only changing 1 or 2 things is quite helpful

Agreed 👍 let's hold off from merging until @caiw is with us on Friday.

[caiw]

still not convinced about requiring certain parser options to be passed when running, especially as (I'm assuming) this will be run on the login nodes running random functions on sensor space, having a baseline and then only changing 1 or 2 things is quite helpful (for me at least)

Happy to discuss this!

If you're talking about removing arguments' default values, thereby requiring them to be supplied at the cli, my thinking was as follows:

It's better to have the gridsearch functions themselves, and the cli invoker, be agnostic to any one specific analysis project, and more just library code. Certain things can have default values if they make sense as a generic default for all (i.e. "we recommend this" or "you'd need a reason to change this", like --seconds-per-split or --audio-shift-correction), but if it's just a project-by-project save-some-typing thing like --function-name or --inverse-operator-dir, then it makes sense to store the default values in a different project-specific script.

Otherwise we have the situation where:

1. it's not clear which arguments are true defaults/recommendations (--audio-shift-correction) and which are just conveniences (--data-path, or the [currently unused] participants variable in run_gridsearch.py)
2. changes to these defaults within one specific project will either end up getting into the codebase, which will can cause downstream conflicts for other users, or else be rendered meaningless as time goes on

When running repeated analyses for a given project project, one can have a separate shell/python script which calls the gridsearch function, providing default params, and allowing rapid re-running only changing one or two things. E.g. I might have a gridsearch_swipe_pitch_dataset_3.py which contains paths and defaults for this project, and just calls the toolbox as a package.

I'm not religious on these points, but that was my thinking.

If I've misunderstood the point: sorry, and happy to discuss what you actually meant :)

[neukym]

We have agreed to remove requirements (and reinsert defaults) - but that in the long run, everything will be done via config files, but with CLI argument overrides (documented in #137).

[caiw]

When we've tracked down the bug with the mismatched hemisphere sizes, I'm happy for this to be merged in

[neukym]

Unfortunately this doesn't work on the queue:

'/usr/bin/bash: line 1: cd: /var/spool/slurm/job3347806: No such file or directory'

Switching back.