Overall organization of the project: about sub-packages

jcohenadad commented 4 years ago

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 commented 4 years ago

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

jcohenadad commented 4 years ago

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 commented 4 years ago

I like it!

jcohenadad commented 4 years ago

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

+b0map
 |- @phase_difference

+b1map
 |- @blabla

+b0shim

vote with thumbs up/down

gaspardcereza commented 4 years ago

There is also +mappers in +B0map

jcohenadad commented 4 years ago

There is also +mappers in +B0map

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

gaspardcereza commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

jcohenadad commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

i definitely think we should include a wrapper for prelude

jcohenadad commented 4 years ago

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

load_niftis → load_nii

jaystock commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

jcohenadad commented 4 years ago

i've updated https://github.com/shimming-toolbox/shimming-toolbox/issues/148#issue-643281411 to use full snake_case, and implemented https://github.com/shimming-toolbox/shimming-toolbox/issues/148#issuecomment-651928828

po09i commented 4 years ago

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 commented 4 years ago

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

shimming-toolbox / shimming-toolbox-matlab

Overall organization of the project: about sub-packages #148

Context

Potential solution