Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom values for brain_phantom #484

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

gsahonero
Copy link
Member

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.

Copy link

codecov bot commented Sep 22, 2024

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

Impacted file tree graph

@@            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
Copy link
Member

cncastillo commented Sep 23, 2024

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.

Changing from `custom_values` to `tissue_properties`
@gsahonero
Copy link
Member Author

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

@cncastillo
Copy link
Member

cncastillo commented Sep 23, 2024

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.

KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
KomaMRIPlots/src/ui/DisplayFunctions.jl Outdated Show resolved Hide resolved
KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
KomaMRICore/src/simulation/SimulatorCore.jl Outdated Show resolved Hide resolved
KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
@gsahonero
Copy link
Member Author

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.

KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
KomaMRIBase/src/datatypes/Phantom.jl Outdated Show resolved Hide resolved
@cncastillo
Copy link
Member

It looks a lot better now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants