Custom values for brain_phantom

[gsahonero]

It would be useful to be able to change the default values. For example, when dealing with T1 or T2 changes due to different magnetic field strengths.

The changes on brain_phantom2D and brain_phantom3D allow the user to define the desired values if passed as an argument.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.69%. Comparing base (00a8d8a) to head (4020c3d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
- Coverage   90.77%   86.69%   -4.08%     
==========================================
  Files          54       54              
  Lines        3002     2991      -11     
==========================================
- Hits         2725     2593     -132     
- Misses        277      398     +121     

Flag	Coverage Δ
base	86.98% <100.00%> (-0.09%)	⬇️
core	72.95% <ø> (-20.25%)	⬇️
files	91.69% <ø> (ø)
komamri	93.98% <ø> (ø)
plots	92.95% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
KomaMRIBase/src/datatypes/Phantom.jl	83.69% <100.00%> (-0.60%)	⬇️

... and 9 files with indirect coverage changes

[cncastillo]

I think this is a good idea, but how this is implemented adds too many lines and complexity (you repeat the same code twice). That is why the code coverage fails, as your not adding a test for the other if branch.

I would suggest having default values in custom_values (tissue_properties I think is a better name) that give exactly what we have now if not specified.

If the user specifies tissue_properties, you could merge the custom properties with the new ones so that the user can define just a few tissues. A dictionary would be suitable for this.

I think this could be done without adding so many lines.

[gsahonero]

What do you think about it now? I removed those lines.

[cncastillo]

I think the default properties could do something similar to the simulation parameters, and have a default_brain_tissue_properties function. And the user defined parameters are merged to that. As it doesn't make sense to be defined twice (one in 2d and one in 3d).

Also it makes more sense that the keys for the dictionary were the tissue names, and that inside you can specify the per tissue properties in some documented order. As the other way around the user needs to remember which tissue was which and it's prone to errors. It will be more verbose but easier to understand, and enable to modify just some tissues.

[gsahonero]

Sorry about those last commits, the last one is the good one.

In summary, I wrote a default_brain_tissue_properties that is being called for creating the 2D and 3D brain phantom. That way, redefinition is avoided. Also, I changed the definition of tissue properties and now the names are being used. I hope those changes may address the suggestions.

[cncastillo]

It looks a lot better now!

[cncastillo]

Ok I had to merge another branch before and now this has merge conflicts. Could you solved them so I can merge this?

[cncastillo]

Nice. Apply these last minor suggestions, and I will merge them (some spaces and new lines are being added).

[cncastillo]

Just realized that some parts of the code were not completely ok. They are also minor.

[cncastillo]

Hi @gsahonero when do you think you could finish this?

[gsahonero]

Sorry for taking this long. I hope the changes address the requested modifications.

[cncastillo]

Could you try one last thing?

The sorting and use of tissue_texts, does not make a lot of sense. It is better if tissue_labels is also a dictionary (Dict("CSF" => 23, ...)), and we index it by the default_properties keys.

You should be able to do:

for i=1:Nproperties
    for (tissue, property) in tissue_properties
        data_properties[i, :, :, :] += (labels .== tissue_labels[tissue]) * property[i]
    end
end

Also, don't forget the spaces between the *!