search

sampsapursiainen / zeffiro_interface

Interface for using finite elements in inverse problems with complex domains
GNU General Public License v3.0
24 stars 15 forks source link

`fs2zef.sh` improvements #192

Closed mshavliuk closed 1 year ago

mshavliuk commented 1 year ago

Suggestions

  1. The import_segmentation.zef has a legacy format and I didn’t manage to use it for import. Let’s replace it with a new one - I would suggest to use the import_segmentation.zef but without the first line importing electrodes.dat since electrodes are not generated by the script.
  2. Also, let’s add the cp import_segmentation.zef $OUT_DIR/ to the fs2zef.sh so it produces a ready-to-import directory.
  3. Why the project has 3 different fs2zef.sh files? For me it is very confusing even though I spent many hours converting different MRI scans. If we absolutely need to have so many let's at least use different names for them. I would suggest to keep only on and rename it to freeSurfer2zeffiro.sh
  4. It looks like previously the fs2zef.sh was performing the reconstruction with recon-all based on this code fragment https://github.com/sampsapursiainen/zeffiro_interface/blob/4b5132d6f9eb5bf46dfff710c6a6558870aa3597/scripts/scripts_for_importing/mri2mesh/fs2zef.sh#L38-L48 Why that code was disabled? Do we want to make the convertation process more difficult and require users to run recon-all manually? This may lead to unwanted problems and mistakes.
SeSodesa commented 1 year ago

It might be that the execution of recon-all needed to be separated from this script, possibly because multiple different segmentations were generated from the same reconstruction, so it didn't make sense to keep it included. Whatever the reason was, it was done before I started meddling with the script, so I can't say for sure.

We could add an additional Boolean argument to the script, such that if it is set, recon-all is executed as well.

mshavliuk commented 1 year ago

Solved by #193