duneuro integration & files organisation #1
Comments
Funny, maybe, but it doesn't look that easy to manipulate... I'll let you deal with this fancy feature and will only work with the repo brainstorm-tools/bst-duneuro :)
Yes, delete this subfolder bst_addons from this repo and open a PR to push them to the brainstorm3 repo.
Can you make sure it works without any functions outside of brainstorm3 and bst-duneuro folders?
I'll start reviewing all your code after your create the brainstorm3 repo pull request, and we'll see if we need to schedule some more live discussions. |
|
ok, sounds good, |
|
Hey, |
Everything & any contributions are welcome !! |
|
Hi, I've finished the documentation and during the following minutes will push some final versions of the compilation script. It downloads the source code, cleans previous builds (reduces the chances of problems during compilation), builds everything both for windows and Linux and finally copies the binary application files to /bin folder with the compilation date in the name of the app. @ftadel following your idea, I've checked and run the compilation using a new ubuntu virtualbox instance. So I guess anybody could re-do the compilation again. Hope this script is useful once we move on and forget how this all worked. Let me know if you have any doubts. @tmedani 2 things. |
|
Hey there, Thank you, @juangpc for this wonderful work. Just a small comment, is it possible to change the "toto" folder to "brainstorm_app" os something similar?
I have checked, and update it.
done, all the matalb code is moved to a new folder called "matlab" |
@juangpc : for this part, we need to write a new interface that call original Duneuro Matlab binding. |
|
@juangpc I tried to run your script on my WSL/Ubuntu, but it didn't work.
Now the error log: |
|
That's new. something not working. |
|
Hi,
Hope it works. The output will be a file named This means that matlab's call to the executable will need to include the date/version number. I got this idea from the way inverse computation scripts are called (2016 vs 2018 versions, etc...). I think we should enforce some kind of way to verify which version it is being called. We have too many links in this chain and if something is not working we should be able to easily discard sources of the error. This is my intention with the builddate-in-the-name thing. Another possibility would be to have the output of the executable application include the version of the code being executed. Right now, the app will create two separate binary files each storing some matrix. The binary information is preseeded with the following info: |
|
Ah! and toto->brainstorm_app... |
|
Super @juangpc ... I will set up a new virtual machine and check it.
I have just added a way to call the exact 'binaries' from Matlab, assuming that the binaries of the linux and windows are the same (8476ca7#r36958908).
also good, but need more modification on the Cpp code-writer and the MatLab reader. |
|
yes. that is actually a nice idea. and if the server is down we can
automate the unzip into src. all seamless to the indifferent user.
On Fri, Jan 24, 2020 at 5:36 PM Takfarinas MEDANI ***@***.***> wrote:
Super @juangpc <https://github.com/juangpc> .
does it make sense to move the file: src_vault.tar.gz to "config" folder
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1?email_source=notifications&email_token=ACEKMIER6SEZSJ43LKDPSATQ7N3OXA5CNFSM4J4YVMMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ4NDUI#issuecomment-578343377>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACEKMIAKIAL4DHWUNHSUYUTQ7N3OXANCNFSM4J4YVMMA>
.
--
Juan
|
Maybe there are things I didn't follow here, but this does not seem to be a very standard practice, and I'm not sure this is necessary because there is already a check done in Brainstorm itself (and the installation/update of these binaries will be done by Brainstorm in most cases). I added a webhook that calls a PHP script on the neuroimage server that repackages bst-duneuro.zip every time something is pushed to the bst-duneuro repository. So if you update the binaries on the github, you'll be sure that everybody start using this new version immediately. |
|
And regarding the compilation, I still have errors like these. |
|
Hi,
@ftadel I think your problem is that you don't have installed pkg-config. Please do the following.
try calling Sorry for previous not-perfectly working deploys. The last thing I want to do is make you waste your time. I've downloaded everything from scracth and tested it twice and it is working here. Let me know if something's not working again. @ftadel I've updated the readme file so that the packages needed etc... are more clear now. |
Ok, then the user will not be aware about the duneuro binaries updates, I think also he doesn't need it. I think in any case, from the "coding" view, both versions will work. |
|
OK @tmedani and @ftadel I see what you mean:
|
|
At some point Id like to try that git large file storage for the zipped src folder. |
|
Hi, I've been reading about it. git large files support requires all collaborators to have installed the feature, otherwise, they'll just see a reference file not the actual file. Given it is just one file, and that the amount of forks in this repo is going to probably be... let's say, close to zero... I'm dropping my idea of using such a feature of git. Though it is one fine thing to know about. Pd. I've been trying to do a bit of further testing and GitLab is down right now... Good thing we have this vault. |
@juangpc the files are already in the folder
sounds good. |
|
never say never.
On Tue, Jan 28, 2020 at 11:29 AM Takfarinas MEDANI ***@***.***> wrote:
2. . @tmedani <https://github.com/tmedani> can you push all the
development ini files to test/ folder in the repository?
@juangpc <https://github.com/juangpc> the files are already in the folder
3. Once a new feature is developed tested and so forth, it can be manually
copied to bin/ folder as a way to "put it into production".
Is this ok with you people? I understand it is a bit messy but is the
least messy it can get
sounds good.
I think we are not going to change many time these binaries.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1?email_source=notifications&email_token=ACEKMIHBSRRMJZEDFQIO55LRABTPZA5CNFSM4J4YVMMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKEGODQ#issuecomment-579364622>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACEKMIAH2ZT23V5OATT6HTLRABTPZANCNFSM4J4YVMMA>
.
--
Juan
|
|
Hello, @ftadel I updated the bst-duneuro folder, it's ready for your reviews. So, regarding my last modifications, the duneuro binaries will not be copied to the bst temporary folder, but before running the binaries we 'cd' to the \bin\ However, the outputs of the binaries are in the \bin\ folder (the leadfiled matrix and the transfer matrix) Some explanations about the binaries: there are mainly two outputs The LF is used immediately, and the TM is just saved for now.
The TF needs to be computed again, So, in the scenario, where the user changes the cortex (source space) and want to compute again the FEM, we can call this matrix. The duneuro binaries are already programmed to check if there is any TM in the current folder. |
They don't have any way of specifying the output folder?
We had coded optimized things like this for OpenMEEG as well, to reuse intermediate computation steps, but ended up dropping them. It was causing more confusion than it was helping, and it was making the updates of the software more complicated... In practice, most Brainstorm users do not compute different forward models, especially not with different source spaces. If there is something that they compute multiple times, it's the inverse solutions. If they compute different forward models, it is to compare different methods (ie. same source space, but to compare a spherical model with a BEM). I think storing this transfer matrix would be useful almost only for the developers (you, Juan, myself). For other users and a standard processing pipeline, it can bring more trouble than advantages. Maybe it's not necessary to spend time on making this a public standard feature at the moment. |
|
Hi, Regarding the discussion of saving the transfer matrix. I'm sorry to disagree but I do think that it makes a lot of sense to save it. For instance, one of the scenarios where this all might become truly useful is in seeg, where you might want to check different electrode positions before surgery. I think that, since it is already working, I would leave it. Besides, nobody "really needs" to understand the dynamics of it all. Some users will notice the second time you compute a similar model the computation is much faster, but they don't really need to know. Still, let me offer a few ideas:
by "they" and "them" you are actually referring to probably @tmedani and myself 😸 😸 |
|
How big is this transfer matrix?
No, let's avoid packing the memory...
It would be good to have a solution with which 1) we don't have to copy the binaries 2) we don't need to save the output in the binary directory (in the compiled version, the binary folder will be embedded in the brainstorm distribution and possibly read-only). So yes, an output folder option would be good.
Oops, I didn't realize that this step was coded by you guys, I thought it was coming from the duneuro people :) |
Not too big, its size is [Number of Sensor X Number of Source] From this answer, I think I wrote something wrong in my previous post, sorry guys!.
This is already solved in the last version of bst-duneuro code.
We will updates the binaries asap and add an output directory. |
|
Hi, I've been doing some testing the cpp app seems to work. Nice input Tak. Yes I now have a way to specify the output folder within the .ini file But I guess, now my question is. How do you want to use it within brainstorm? Or in other words... we can:
By the way, both relative and absolute paths will work in both options. let me know which one you prefer and I'll compile and push. |
|
Hi @juangpc , For your question
As it is implemented now, when it's needed, |
|
ok thanks
how do we transform .brainstorm/tmp into something the cpp app will
understand?
the app only knows either absolute paths or relative to from where the app
is running from, which in our case will be from where matlab is calling it
from. and that info needs to be inside the text ini file and correct for
every brainstorm/matlab installation.
On Fri, Jan 31, 2020 at 5:43 PM Takfarinas MEDANI ***@***.***> wrote:
Hi @juangpc <https://github.com/juangpc> ,
This is super! bravo.
For your question
How do you want to use it within brainstorm?
As it is implemented now, when it's needed,
bst-duneuro toolbox will be download and installed on
.brainstorm\
and when the FEM is called, all the temporary folders (dipole, electrode,
mesh ....) will be saved to
.brainstrom\tmp
for now, only the binaries outputs are on
bst-duneuro\bin
but with your modification, they will be also moved to the tmp folder,
which will keep the bst-duneuro/bin clean
so, we need just to add on the ini file the value of the outputfolder like
this :
outputfolder = .brainstorm\tmp
Then we are done with this :)
@ftadel <https://github.com/ftadel> do you agree?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1?email_source=notifications&email_token=ACEKMIAKWDSNEFEBF7ETRHDRASZTNA5CNFSM4J4YVMMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKQLQMI#issuecomment-580958257>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACEKMIAQHGNHKTCD4DY2QK3RASZTNANCNFSM4J4YVMMA>
.
--
Juan
|
|
Hi @tmedani, sorry I wasn't understanding you. But as I understand it right now, the problem is in fact inside matlab, not the cpp. The way matlab handles the spaces... I'll show you the fix so you can solve the issue yourself, I don't want to mess around with the scripts while you're working with them. >> system(callStr1) >> system(callStr2); >> system(callStr3); >> system(callStr4); -- so... >> system(callStr5); >> system(callStr6); Both callStr5 or callStr6 will work JUST FINE! |
|
By the way, we need to update the help message! |
Super Juan .... thanks for the super-review.
indeed... still a lot to do... |
|
Hi all. Its been quite painful to adapt the code to run in a mac. I'm writing this to inform you and also as a reminder for myself in case I need to go through this fantastic experience of adapting code designed for a different OS, again. There were some random errors and some need for static allocations, and other things. I've renamed the binary files, for linux64, win64 and mac64. The first call seems to be working. The second call "sometimes" produces this segfault, right before closing. I figure it might be related to the ownership of some smart pointer. linux and windows compilation can be done with the same linux machine automatically (see help). Take care @tmedani and @ftadel (and anybody else out there reading). |
|
Windows, Linux and Mac binaries can be found in the |
|
Thank you @juangpc for this detailed report, and for the lot of efforts that you put to make it work... If I understand, I have to updates the call for the binaries from bst_duneuro... I will updates this asap.
Thank you... take care and stay safe all of you ... |
|
HI @tmedani , yep. If you look in /bin folder you will see 3 binaries now for all mayor x64 OS. We agreed (I trhink) to keep these 3, right? The code is practically unchanged between mac and linux, but the binaries are different. There were some problems with a pointer and a few other minor things. Also, another issue was that macos "sed", is surprisingly not equivalent to gnu's. It took a while to figure this out. Apart from that, the code compiles now. And all the compiling scripts too. There is this issue, still. It doesn't appear in windows, nor linux. It didn't appear in high sierra but for some reason I am seeing this segmentation fault in Catalina. I just can't figure it out. And, valgrind is... as mentioned, still not ready for Catalina, so:
|
|
@tmedani @juangpc We need to split what is currently on brainstorm-tools/bst-duneuro in two:
Two possible solutions:
What do you think? |
|
Hi there,
Why not running some kind of: platform = mexext;
if strcmp(platform,'mexmaci64')
% Code to run on Mac.
% download mac version of binary
binUrl = 'https://github.com/brainstorm-tools/bst-duneuro/blob/master/bin/bst_duneuro_meeg_mac64?raw=true';
filename = 'bst_duneuro_meeg';
outfilename = websave(filename,binUrl);
system('chmod +x bst_duneuro_meeg');
elseif strcmp(platform,'mexa64')
% Code to run on Linux platform
% equivalent linux code
elseif strcmp(platform,'mexw64')
% Code to run on Windows platform
% equivalent win code
else
disp('Platform not supported')
endAs usual @ftadel I don't intend to mind the way you like to write bst or matlab code in general. What I want to do is learn and improve my cs knowledge. That is why I'm proposing this snippet. Is it a bad idea? (I am assuming it is) 😅 But why? instead of a webhook whatever we do on our dev side, we just make sure the name of the bin file is that one, and therefore it will always be there. Personally, I would definitely discard creating another repo. |
|
I forgot to mention other things you need. So strictly necessary is:
|
great, please let me know when you will start.
in order to keep the actual development (DTI/anisotropy) I prefer the option 2, @ftadel does it work for you?
agree also, we have already too much for this project.
How this is possible?
I have Linux and windows but nor mac, I will try to test on these machines asap. also, we will have at least one student from Carsten's group who will start working with this tool and we can have their feedback. |
Yes, please do.
Where do you want to put this code? I don't understand why you want to re-download the macos binaries, and only the mac binaries, while there is already a mechanism in place that download the three executables at once and keep them updated. |
|
Hi,
The "only mac" was because that's the only case I coded, but I meant to show the snippet as an example. Just to do the same for all 3 major platforms. Regarding the other mechanisms in place:
I understand from the code, and from this message, you shared a few weeks ago (previous paragraph). That every time something is pushed to the repo, a copy of the binaries and only the binaries is saved in neuroimage server, from where bst users will then download the actual apps. Ok, questions:
|
No. When something is pushed to the bst-duneuro repo, it calls a packaging script that clones and zips the ENTIRE repository and makes it available on the Brainstorm download page: When Brainstorm needs to get bst-duneuro, it gets from there, with a different PHP script but reading the same .zip file:
Once zipped, each of this binary executable is ~2Mb... I think it's ok to download 4Mb extra for the other OS. I didn't think these 4Mb were a good reason to get to the trouble of generating 3 different packages.
Automatic installation from Brainstorm, which is not possible otherwise without having git installed on the system. Possibility to disconnect the "released package" from what is happening on the git repo, if we need it (and we will, because we will generate a package that includes only a part of the repo, excluding the dev folder).
None, for the general user.
Sure. |
I'm not understanding you. You only need an internet connection and use urlread/webread/websave to download a file. You do not need git installed. And the disconnection from what is happening in the git repo is done by fixing a hash value in the actual url. But, anyhow, if this is how you want it to work, that is just fine too. Thanks for the explanation anyway. Regarding the If your answer is no. You want a /dev folder. Could you guys push your changes so that I can create it, avoiding merge conflicts? |
|
Yes, we could leave everything the way it is now if simpler for you, but it is not very intuitive to understand what is the "brainstorm plugin" (matlab scripts + compiled binaries) and what is "your development environment" (everything else). These are two different subprojects of this repository. But ok to avoid any additional work for you. Does this work for both of you? |
|
Yes, this works. Thank you very much. |
|
@tmedani Ok for you as well? |
|
@ftadel , yes agree, there some some nonuseful functions on root. |
|
FYI: I modified the packaging script (which is called when something is pushed to the bst-duneuro repo), so that it zips two subfolders only: /bin and /matlab. Everything else is just ignored. The "brainstorm plugin" which is downloaded automatically by Brainstorm when needed (also available at the bottom of the download page: https://neuroimage.usc.edu/bst/download.php) is now only 9Mb, instead of 120Mb before. |
|
Thanks! |
|
Hi @tmedani and @ftadel ,
Now the catch... I would need my Linux box which is at my desk, where I'm not supposed to go... I can remote desktop but it is a bit laggy. The linux is actually virtual machine, so it would imply to remote to a windows and then into a virtualized linux... a bit of a pain to code that way. So. Do you think this is needed, or maybe it is not a priority? |
What do you mean? Nothing in the /matlab folder is actually needed to compute a FEM forward model with duneuro?
I'm not sure what it entails. Maybe this is more a question for @tmedani? |
No, sorry for the confusion. This might be a bit into the details, but here it is... Dune is the linux only project that we are using for fem. Actually a sub-project of that one called DuNeuro. This subproject can run in two different ways now: as a c++ application and as a mex file to be used within matlab api... with a catch... it only works for matlab's linux version... This last one, the matlab version is the one we're using to build our non-matlab exe file. Why? ... you might ask. Because it didn't work otherwise... But now I think I know how to make it work. That was my message. |
|
Hey, @ftadel this is related to the hidden nightmare of the compilation. @juangpc this is great ... I know that it's possible but never manage to do it... Then, with this, it would be better to avoid to clone the matlab module and move all the brainstorm_app from duneuro_matlab to duneuro module? we can also keep an option that creates the mex file ... for advanced users who want to use Linux Let's discuss it ... when you can. |
|
yes. modifying some paths. and the cmake tool. we can also keep an option that creates the mex file ... for advanced users who want to use Linux Given this repo is for brainstorm use... I would say that it is better to stay away of the mex api. However, if you guys want to duplicate the fem functionality for the case of linux based brainstorm users, the function would end up being very similar, although there would be no need to read the resulting binary file... because matlab would have access to the memory assigned to duneuro. |
|
great ... let have a look ... later
no need to change all the process, it could be faster ... but how much? however, changing the brainstorm_app folder to duneruo_module could be nice. |
|
Ok. Will do. (don't know when, though).
Juan
…On Fri, Apr 3, 2020 at 2:51 PM Takfarinas MEDANI ***@***.***> wrote:
great ... let have a look ... later
However, if you guys want to duplicate the fem functionality for the case
of linux based brainstorm users, the function would end up being very
similar, although there would be no need to read the resulting binary
file... because matlab would have access to the memory assigned to duneuro.
no need to change all the process, it could be faster ... but how much?
it needs more investigation and more work...
also the user needs to compile the duneruo for its self... maybe useful
for developers
however, changing the brainstorm_app folder to duneruo_module could be
nice.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACEKMIHFP3YZT3UUVBWVRMDRKY437ANCNFSM4J4YVMMA>
.
|
|
@juangpc FYI: I tested the compilation of the bst-duneuro binaries following your instructions in the README on Windows10 and WSL/Ubuntu18 and both worked at the first attempt! Nice |
|
😅 |
Hey @ftadel & @juangpc,
Sorry if you are getting many spams due to my recent activities in this repo.
I'm still learning how to use git and its fabulous features.
So to keep all the tracks on the code, this repo is connected to another repo:
(I discovered this funny git option 'submodule')
@ftadel here is the invitation, it is a toolbox called "duneuro interface",
where I collected the main functions to run and test duneuro from matlab.
@ftadel I followed your recommendation Nb 2, and I uploaded all to the bst_duneuro toolbox.
At the end of this message, tell me if I need to apply the recommendation Nb 1.
all the functions related to brainstorm integration are, hopefully, in the folder bst-duneuro.
otherwise, they are somewhere in the other folder named "toolbox".
We need just to move them to "bst-duneuro" folder.
Then, in this repo, I added a temporary folder, "bst_addons", where I put the original functions of brainstorm that I modified, listed in this picture (this image is also in this folder)
With all these together, I was able to run the EEG, MEG and combined MEG/EEG.
Also, please, don't hesitate to change, comment, criticize, and of course, modify these functions.
this is just a first attempt to make duneuro working from brainstorm.
I have still a lot of improvement that I notified on the code, but you know, code optimization has no limit.
@ftadel I added a lot of comments on the code, as a TODO, where either I didn't know how to do it, or the way how I did it is not the best one.
If you want, we can also plan a bluejeans/skype discussion in order to clarify all these.
Thank you & A+
Takfarinas
The text was updated successfully, but these errors were encountered: