Nirstorm/nirstorm

Move to the Brainstorm plugin manager?

Closed this issue · 17 comments

ftadel commented

Cross-posting the issue from the Brainstorm repo for including all the developers:
brainstorm-tools/brainstorm3#388

Edouard2laire commented

Hello Francois,

Since the PR has been merged in Brainstorm, I think we can close this issue.

One last question for you and @thomas-vincent, What should we do about the previous installation scheme? (eg nst_install script) ? Should I remove them from the current repository?

Can you confirm me, also, that Nirstorm is currently included in the compiled version of Brainstorm?

thomas-vincent commented
ftadel commented

One last question for you and @thomas-vincent, What should we do about the previous installation scheme? (eg nst_install script) ? Should I remove them from the current repository?

This is not useful for Brainstorm, so I don't have any opinion.

Can you confirm me, also, that Nirstorm is currently included in the compiled version of Brainstorm?

It should. All the Nirstorm code is compiled and included in the package.
I've just re-compiled it with fixes so that the Nirstorm processes appear in the Pipeline Editor window:
brainstorm-tools/brainstorm3@3bdbed0

There might be other issues, but since I don't work with NIRS data at all, it's difficult for me to test it.
Can you please try it, and make sure that all your standard tools work well with this new compiled package?

ftadel commented

And close the issue when you validated that it works.

ftadel commented

One more problem to solve: the co-dependency of the nirstorm and brainentropy plugins.
Now we moved the MEM functions as a separate plugin, I'd like to move be_main.m and panel_brainentropy.m to the brainentropy github repo:

Therefore process_nst_cmem.m and process_nst_cmem_fusion.m GetDescription() function crash when the brainentropy plugin is not installed. I see two options for this problem:

  • you rewrite the beginning of the process with something like: if ~exist('be_main',2), sProcess.Index=0; end to hide them when the brainentropy plugin is not installed.
  • you make nirstorm dependent on brainentropy, so that installing nirstorm always causes the installation of brainentropy.

For both options: set sProcess.options.mem.Value=[] instead of be_main(), since panel_brainentropy already defines its own default values.

Edouard2laire commented

Hello

What do you think of this :

% Options: MNE options
sProcess.options.mem.Comment = {'nst_MEM_option', 'Estimation options: '};
sProcess.options.mem.Type    = 'editpref';
sProcess.options.mem.Value   = [];

with nst_MEM_option.m

function [varargout] = nst_MEM_option(varargin)
    % Install/load brainentropy plugin
    [isInstalled, errMessage] = bst_plugin('Install', 'brainentropy', 1);
    if ~isInstalled
        varargout={}
        return;
    end
    varargout=panel_brainentropy();

end

so Best is installed when you click on the option button.

ftadel commented

No: The GetDescription() functions are called often and must be as fast and as self-contained as possible. Calling external functions from there is not recommended, and installing plugins from it is impossible.

If you want the two plugins nirstorm and brainentropy to be linked, this is exactly what RequiredPlugs does in the plugin definition.
PlugDesc(end).RequiredPlugs = {'brainentropy'}

Edouard2laire commented

nst_MEM_option is only called if you click on the button; no ? i tried to put a disp inside; and its not displayed if you don't click on the button. Meaning the function is not called

Edouard2laire commented

If you want the two plugins nirstorm and brainentropy to be linked, this is exactly what RequiredPlugs does in the plugin definition.
PlugDesc(end).RequiredPlugs = {'brainentropy'}

ok. this might be the better option. it's just that not a lot of people will be using MEM when using Nirstorm. So i am not sure it's ok to 'force' everyone that is using nirstorm to also dowload Best; but guess that's ok :) Can I ask you to make this change in the brainstorm repo ? :)

ftadel commented

nst_MEM_option is only called if you click on the button; no ? i tried to put a disp inside; and its not displayed if you don't click on the button. Meaning the function is not called

This is bad coding practice. Please chose between:

  • making brainentropy plugin a requirement for installing the nirstorm plugin (heavier)
  • making some nirstorm processes available only when brainentropy is installed. For example, you can grey them out by defining an empty InputTypes list when ~exist('be_main',2), and/or hide the options and display a message "you must install the brainentropy plugin to enable this process" with option type label.
ftadel commented

ok. this might be the better option. it's just that not a lot of people will be using MEM when using Nirstorm. So i am not sure it's ok to 'force' everyone that is using nirstorm to also dowload Best; but guess that's ok :) Can I ask you to make this change in the brainstorm repo ? :)

Second option with having a label asking the installation of an extra plugin is lighter.
It depends how much you think a typical Nirstorm user would use the MEM functions: a lot=option 1, very little=option 2

Edouard2laire commented

ok. I will use the second option. Should make the change latter today in the nirstorm repository. May i ask you why the proposed option is a bad coding practice ?

Edouard2laire commented

Hello @ftadel,

I just realized that when nirstorm is loaded, we cannot download any other plugin. What could be the reason?

ftadel commented

May i ask you why the proposed option is a bad coding practice ?

I'm trying to separate as much as possible:

  1. the package management (plugin installation),
  2. the batch management (pipeline editor)
  3. the interactive environment of the user

The batch management should be something completely static, independent from the user's local installation. One goal is to be able to generate scripts that are sent to a distant computation node. Therefore it should not be the batch editor that installs the plugin, but the batch execution (either the calling script or the process itself in the Run() function).
The function GetDescription() deals with metadata only, it operates at the level of the batch editor. The Run() function is responsible for the computation and should deal with missing parts of the processing environment, and install the missing plugins.

If you want to be really formal: brainentropy is a requirement for nirstorm.
If you don't want to enforce this, then find solutions that disable some of your functions when they are not available.
But you should not try to implement any behavior that cannot be reproduced by a script (installing a dependent plugin manually from a .m script is possible, installing it automatically from the Run() function too, but not "installing something when a button is pressed in the pipeline manager window").

I'm not sure I managed to be very clear here... but this bothers me because this is the kind of non-scriptable behaviors we're trying to remove from the software.
We have other solutions, so let's go another route :)

ftadel commented

I just realized that when nirstorm is loaded, we cannot download any other plugin. What could be the reason?

I can't reproduce this problem...

Edouard2laire commented

One more problem to solve: the co-dependency of the nirstorm and brainentropy plugins.
Now we moved the MEM functions as a separate plugin, I'd like to move be_main.m and panel_brainentropy.m to the brainentropy github repo:

Therefore process_nst_cmem.m and process_nst_cmem_fusion.m GetDescription() function crash when the brainentropy plugin is not installed. I see two options for this problem:

  • you rewrite the beginning of the process with something like: if ~exist('be_main',2), sProcess.Index=0; end to hide them when the brainentropy plugin is not installed.
  • you make nirstorm dependent on brainentropy, so that installing nirstorm always causes the installation of brainentropy.

For both options: set sProcess.options.mem.Value=[] instead of be_main(), since panel_brainentropy already defines its own default values.

Actually, it doesnt't work. it seems GetDescription is not called after Brainstorm is started. So if Best is not in the path when brainstorm is started then you cannot use Best so you need to load Best and then restart Brainstorm.

Issue is also that if you quit matlab, Best is removed from the path. so Every-time, we need to open brainstorm, add Best, and restart Brainstorm ...

For both options: set sProcess.options.mem.Value=[] instead of be_main(), since panel_brainentropy already defines its own default values.

Also it seems that panel_brainentropy doesn't initialize everything

ftadel commented

Actually, it doesnt't work. it seems GetDescription is not called after Brainstorm is started. So if Best is not in the path when brainstorm is started then you cannot use Best so you need to load Best and then restart Brainstorm.

You could make it load automatically just like nirstorm, by adding:
PlugDesc(end).AutoLoad = 1;
Does that solve the issue?

Also it seems that panel_brainentropy doesn't initialize everything

Maybe this is something that could be fixed in panel_brainentropy.m directly, to make it more self-contained?
(panel_brainentropy could call be_main.m, as they are in the same folder)