Overall organization of the project: about sub-packages

[jcohenadad]

Context

As we are currently refactoring the project, we are developing a lot of image-processing methods under +imutils.

We should anticipate how/where all functions/packages of the toolbox will be organized, before it is too late.

Potential solution

The following is a suggestion for packages/functions:

+imutils  # Various image processing stuff (regridding, segmentation, etc.)
 |- @read_nii
 |- @load_nii
 |- @get_nii_coordinates
 |- @dicom_to_nii

+unwrap
 |- unwrap_phase
 |- +unwrappers
     |- @sunwrap
     |- @prelude
     |- @qgu
     |- @abdul_rahman

+b0map
 |- mapping
 |- +mappers
     |- @phase_difference
     |- @multiecho_linfit

+b1map
 |- @blabla

+b0shim
 |- +coils
 |   |- +greg
 |
 |- +parts
 |- +compute

NOTE: The example above is a WIP, please everyone feel free to edit it and/or comment
2020-06-22 14:37:27: still missing: aux_harware, Ui, misc, example (probably not package), external, tests

[po09i]

I think we should also have a maximum depth as having 3 folders deep is already quite long: imutils.b0.mappers.phase_difference()

[jcohenadad]

so how about this:

+imutils  # Various image processing stuff (regridding, segmentation, etc.)

+unwrappers
 |- @sunwrap

+b0mapping
 |- @phase_difference

+b1mapping
 |- @blabla

+b0shimming (changed for consistency with +b0mapping, we could also go with +b0map)
 |- +coils
 |   |- +greg
 |
 |- +parts

[po09i]

I like it!

[jcohenadad]

for brevity, should we instead to without the "ing"? i.e.

+b0map
 |- @phase_difference

+b1map
 |- @blabla

+b0shim

vote with thumbs up/down

[gaspardcereza]

There is also +mappers in +B0map

[jcohenadad]

There is also +mappers in +B0map

the idea is to get rid of these additional layers, i.e. imutils/b0/mappers → b0map

[gaspardcereza]

the idea is to get rid of these additional layers, i.e. imutils/b0/mappers → b0map

So the field function that calls the different mappers would be in the same folder as the mappers themselves ?

[jcohenadad]

the idea is to get rid of these additional layers, i.e. imutils/b0/mappers → b0map

So the field function that calls the different mappers would be in the same folder as the mappers themselves ?

there is no more "mappers". Here is how it would look like:

+b0map
 |- @phase_difference
 |- @another_fieldmapping_method

+b1map
 |- @blabla

+b0shim

[gaspardcereza]

there is no more "mappers". Here is how it would look like:

+b0map
 |- @phase_difference
 |- @another_fieldmapping_method

+b1map
 |- @blabla

+b0shim

So where do we place the function that calls phase_difference or the other field mapping methods ?

[jcohenadad]

So where do we place the function that calls phase_difference or the other field mapping methods ?

how about under +b0map? The call would be:

b0map.mapping(@phase_difference)

[gaspardcereza]

Ok I'll just look for a way to not take it into account when I display the different methods available in the error message.

[jcohenadad]

Ok I'll just look for a way to not take it into account when I display the different methods available in the error message.

you still have the option of a dict if there is no other elegant solution

[rtopfer]

above plan sounds good to me. a few (likely obvious) comments/suggestions:

• while i agree we should be careful about overdoing subpackages, max~3 should be treated as a guideline rather than a rule: whatever is most amenable to a maintainable code base should be favored over the minor inconvenience of adding another layer to the folder structure. E.g. @gaspardcereza maybe what you're working on could be an exception?

• code dependencies between packages, when present and when possible, should follow the dir/package structure (i.e. top package is the most general, with subpackages becoming increasingly specific). likewise, there will be exceptions.

• i wouldn't worry too much about horizontal vs. hierarchical package structures since subpackages don't actually create dependencies—they do, however, provide a nice way of hinting at dependencies through the folder structure.

main/general point, as w/everything else: Weigh the pros vs. cons for maintainable code (i.e. logical, readable, concise, organized, etc.—in a word: comprehensible)

[gaspardcereza]

I'd like to put all the unwrappers in +unwrappers. Can you give me a list of the different unwrappers available @rtopfer ?

[jcohenadad]

ok, i've updated the "final solution" in the first comment. Please all: if you could fill the structure as much as possible with the already-existing functions (and the ones we anticipate to create), it would be great. We should have an overview of the overall architecture before implementing it.

[rtopfer]

I'd like to put all the unwrappers in +unwrappers. Can you give me a list of the different unwrappers available?

Nominally, the list consists of the different cases called in the switch statement in MaRdI.unwrapphase but what's actually "available" is more complicated since 2 of the options require separate installations: prelude requires FSL, and the abdul-rahman method requires compiling the source code (@kousu mentioned it can be tricky to ensure it compiles correctly across systems). So, apart from sunwrap (and the quality-guided method?), which are MATLAB based, I don't know what unwrappers we want to include/support, which is why I suggested trying to keep that processing step as open as possible (e.g. by using function handles)...

[jcohenadad]

i definitely think we should include a wrapper for prelude

[jcohenadad]

i suggest we use single convention for "nii" here:
|- @read_nii
|- @load_niftis
|- @get_nii_coordinates
|- @dicom_to_niftis

load_niftis → load_nii

[jaystock]

I have wrapper Matlab code for BET and PRELUDE that I can upload if it would be helpful. The tricky part is making that the correct FSL environment is set so that Matlab can find FSL on the machine.

[gaspardcereza]

To keep track of what has been discussed during this week's meeting: we decided to go with sub-packages (b0map.mappers & unwrap.unwrappers) to store the different mapping and unwrapping algorithms and easily fetch them when we need to display the available functions to the user.
We also have to make a list of all the mapping and unwrapping functions available and add them to these sub-packages (right now there is only sunwrap and phase_difference).

[jcohenadad]

@gaspardcereza could you pls update the prototype (at the top of this thread) with the latest decisions? thx

[jcohenadad]

i've updated shimming-toolbox/shimming-toolbox#148 (comment) to use full snake_case, and implemented shimming-toolbox/shimming-toolbox#148 (comment)

[po09i]

Here are other functions that need to be worked on to continue the refactoring :

+unwrap

• @rescale_img (MaRdI reference method)

+misc

• @reslice_image (reference MaRdI method)
• @scale_phase_to_frequency (reference MaRdI method)
• mask
  • @segment_spinal_canal (reference MaRdI method)
  • @set_mask (reference MaRdI image)
  • @define_mask
• +pmu
  • @read_pmu
  • @interp_pmu (reference MaRdI method)
  • get_acq_times

+b0map

• @extract_harmonic_field (FieldEval reference method)
• @get_riro (FieldEval reference method)
• @map_field (FieldEval reference method)
• @model_field (FieldEval reference method)

+b0shim/+compute/

• @siemens_basis
• @spherical_harmonics

[gab-berest]

I'll check the PMU part between code review. It will help me to better understand the project.