Nirstorm / nirstorm

Brainstorm plugin for fNIRS data analysis
GNU General Public License v3.0
34 stars 11 forks source link

Improve file organization #90

Closed Edouard2laire closed 4 years ago

Edouard2laire commented 5 years ago

Hello,

Currently, we have 78 files in the bst_plugin folder which make it difficult to find a specific process.

So, i suggest we adopt the same folder convention as brainstorm : bst_process/ |-- core/ contains the main process ( bst_call,bst_process,nst_run_bst_proc and bst_error) |-- IO/ all the input/output process (nst_request_files, nst_get_bst_func_files, nst_save_figure, import_event...) |-- math/ for all the mathematical computation ( knn-research, voronoi diagram) |-- misc/ contains all the utility process ( nst_filter_table, nst_get_local_user_dir, ...) |-- preprocessing/ contains all the pre-processing process (detect bad channels, filtering, Hb computation) |-- GLM/ all the GLM process |-- inverse/ and Forward/ for the inverse and forward computation

What do you think of this organization ? if it's ok for you, I make a draft of what the folder will look likes using this organization.

Best regards, Edouard

thomas-vincent commented 5 years ago

Hello,

Good idea but would trigger some work... There would be the installation script to check/adapt as well as the MANIFEST files. I think the easiest is to stick with a flat one-directory organization but do some renaming to have clearer and more homogeneous file names. Let's work on some conventions to name files.

On Mon, May 6, 2019, 14:35 Edouard Delaire, notifications@github.com wrote:

Assigned #90 https://github.com/Nirstorm/nirstorm/issues/90 to @thomas-vincent https://github.com/thomas-vincent.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/Nirstorm/nirstorm/issues/90#event-2321814239, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE2GDT54JCGLZE7KULWDRTPUB3ATANCNFSM4HLCL5BQ .

Edouard2laire commented 5 years ago

I am not sure the manifest is going to be a problem as we can specify a path. I think changing 'process_nst_glm_fit.m' to 'GLM/process_nst_glm_fit.m' should work ? no?

thomas-vincent commented 5 years ago

I think brainstorm does not catch processes if they are not directly in the .brainstorm/process folder. It does not seem to support subfolders. I just tried to create a dummy process in my folder .brainstorm/process/test and I could not see it in the process list.

The installation script (nst_install) transposes the relative paths from the source to the target folder, meaning that bst_plugin/GLM/process_nst_glm_fit.glm would be installed as .brainstorm/process/GLM/process_nst_glm_fit.m. It would not be seen by brainstorm then. We would have to change the installation specification, so that regardless of the subdirectories, all files end up directly in .brainstorm/process.

As it's only a convenience for developers, I would not vote for that kind of extra work.

Again, sorting out file naming should do it:

We can write a guideline wiki page to settle on this.

Best,

Thomas.

On Mon, May 6, 2019 at 3:59 PM Edouard Delaire notifications@github.com wrote:

I am not sure the manifest is going to be a problem as we can specify a path. I think changing 'process_nst_glm_fit.m' to 'GLM/process_nst_glm_fit.m' should work ? no?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Nirstorm/nirstorm/issues/90#issuecomment-489754646, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE2GDQEXDW5QO4QXT6KXR3PUCEYTANCNFSM4HLCL5BQ .

Edouard2laire commented 5 years ago

I agree with you. I have added theses guidelines to the wiki page. As a first draft, i would suggest this : https://pastebin.com/0WMhFewQ

However, we have to be careful when renaming function as renaming the file will also change the way to call the function : calling nst_get_repository_url won't work anymore if we rename the file nst_misc_get_repository_url.m .

Thus this will also break retro-compatibility with previous user script. Do you have any suggestion for that @thomas-vincent ?

Best, Edouard