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

Separate KomaMRI into simpler subpackages #92

Closed
4 tasks done
cncastillo opened this issue Oct 6, 2022 · 4 comments · Fixed by #163
Closed
4 tasks done

Separate KomaMRI into simpler subpackages #92

cncastillo opened this issue Oct 6, 2022 · 4 comments · Fixed by #163
Assignees
Labels
1) high priority enhancement New feature or request

Comments

@cncastillo
Copy link
Member

cncastillo commented Oct 6, 2022

This needs to be done to remove a lot of the dependencies of the UI like Blink, PlotlyJS, etc.

The idea of doing this is to reduce the number of dependencies to the minimum if someone just wants to simulate data.

To replace MRIReco, the KomaMRIBase package should use instead:
MRIBase
-> RawAcquisitionData
-> AcquisitionData
MRIFiles
-> ISMRMRD

Then, the KomaMRI package can have other dependencies.

Not sure if the plots for our datatypes should be included in KomaMRIBase. Maybe also have KomaMRIPlots?

In summary:
├── KomaMRI.jl (depends on Base and UI)
│ ├── KomaMRIBase.jl (least dependencies)
│ └── KomaMRIPlots.jl (separated to include plots only if needed)

We will need to think about the best way to do this.


Subpackages:

@cncastillo cncastillo self-assigned this Oct 6, 2022
@cncastillo cncastillo added the enhancement New feature or request label Oct 6, 2022
@cncastillo cncastillo changed the title Separate simulation base to KomaMRIBase Separate simulation base to KomaMRIBase subpackage Oct 6, 2022
@cncastillo
Copy link
Member Author

cncastillo commented Oct 6, 2022

We need to think what goes where: KomaMRI (UI, All the rest); KomaMRIBase (simulation); KomaMRIPlots (Plots)

  • ArgCheck = "2" <---Used, but not neccessary
  • AssetRegistry = "0.1" <---UI
  • Blink = "0.12" <---UI
  • CUDA = "3" <---Base
  • Distributions = "0.25" <---Base
  • FFTW = "1" <---(?)
  • FileIO = "1" <---IO
  • HDF5 = "0.16" <---IO
  • Hwloc = "2" <---N threads
  • Images = "0.24, 0.25" <---(?)
  • Interact = "0.10" <---UI
  • Interpolations = "0.13, 0.14" <--Base
  • Ipopt = "0.7, 1" <---Example DiffPrepMCMRF
  • JLD2 = "0.4" <---IO
  • JuMP = "0.21, 1" <---Example DiffPrepMCMRF
  • LinearOperators = "2" <---(?)
  • MAT = "0.10" <---IO
  • MRIReco = "0.4.2" <---Recon
  • MathOptInterface = "0.9, 0.10, 1" <---Example DiffPrepMCMRF
  • Mux = "0.7" <---UI
  • PackageCompiler = "1, 2" <---Not used
  • Parameters = "0.12" <---Base
  • PlotlyJS = "0.18" <---Plots/UI
  • ProgressMeter = "1" <---Base
  • Reexport = "1" <---MRIReco/Blink
  • Scanf = "0.5" <---Example DiffPrepMCMRF
  • Suppressor = "0.2" <--Not used

@beorostica
Copy link
Contributor

(needed) CUDA = "3" <---Base
(needed) Hwloc = "2" <---Base
(needed) Interpolations = "0.13, 0.14" <-- Base, (por sacar)
(added) MRIFiles <---Base
(needed) Parameters = "0.12" <---Base
(needed) ProgressMeter = "1" <---Base
(needed) Reexport = "1" <---Base
(added) ThreadsX <--- Base
(needed) PlotlyJS = "0.18" <---Base (Plots), UI

(needed) FileIO = "1" <---Base, IO
(needed) HDF5 = "0.16" <---Base, IO
(needed) JLD2 = "0.4" <---Base, IO
(needed) MAT = "0.10" <---Base, IO
(needed) Scanf = "0.5" <---Base, IO

(needed) AssetRegistry = "0.1" <---UI
(needed) Blink = "0.12" <---UI
(needed) Interact = "0.10" <---UI
(needed) MRIReco = "0.4.2" <---Reco, UI

@cncastillo
Copy link
Member Author

cncastillo commented Nov 24, 2022

For the moment, it was decided to separate the package in:

├── KomaMRI.jl: includes the KomaUI and Reconstruction (dependencies: Blink, Mux, MRIReco, KomaMRICore, KomaMRIPlots, etc).

  • (needed) AssetRegistry = "0.1" <---UI
  • (needed) Blink = "0.12" <---UI
  • (needed) Interact = "0.10" <---UI
  • (needed) MRIReco = "0.4.2" <---Reco, UI

│ ├── KomaMRICore.jl: least amount of dependencies, just simulation, and IO: Pulseq, ISMRMRD, etc.

  • (needed) CUDA = "3" <---Base
  • (needed) Hwloc = "2" <---Base
  • (needed) Interpolations = "0.13, 0.14" <-- Base, (por sacar)
  • (added) MRIFiles <---Base
  • (needed) Parameters = "0.12" <---Base
  • (needed) ProgressMeter = "1" <---Base
  • (needed) Reexport = "1" <---Base
  • (added) ThreadsX <--- Base
  • (needed) FileIO = "1" <---Base, IO
  • (needed) HDF5 = "0.16" <---Base, IO
  • (needed) JLD2 = "0.4" <---Base, IO
  • (needed) MAT = "0.10" <---Base, IO
  • (needed) Scanf = "0.5" <---Base, IO

│ └── KomaMRIPlots.jl: just the Plots, mainly to remove PlotlyJS as a dependency as it could be a heavy package.

  • (needed) PlotlyJS = "0.18" <---Base (Plots), UI

@beorostica Please use these names. All the KomaUI.jl stuff is part of KomaMRI.jl, not KomaMRIPlots. KomaMRIPlots should include the functions defined in DisplayFunctions.jl. For the time being, I would focus on KomaMRICore.

@cncastillo
Copy link
Member Author

cncastillo commented Oct 21, 2023

This issue should not be closed as separating the IO part is necessary @beorostica. Also we will remove CUDA.jl as a strong dependancy with package extensions and allow the use of more backends:

│ ├── KomaMRICore.jl: least amount of dependencies, just simulation
(needed) CUDA = "3" <---Base
(needed) Hwloc = "2" <---Base
(needed) Interpolations = "0.13, 0.14" <-- Base, (Do we need this dependancy???)
(added) MRIFiles <---Base
(needed) Parameters = "0.12" <---Base
(needed) ProgressMeter = "1" <---Base
(needed) Reexport = "1" <---Base
(added) ThreadsX <--- Base
│ ├── KomaMRIIO.jl: IO helper functions: read/write Pulseq, read JEMRIS/MRiLab etc.
(needed) FileIO = "1" <---Base, IO
(needed) HDF5 = "0.16" <---Base, IO
(needed) JLD2 = "0.4" <---Base, IO
(needed) MAT = "0.10" <---Base, IO
(needed) Scanf = "0.5" <---Base, IO

There are some questions that are important:

  • Do we want the phantom examples to be part of the core? (like bran_phantom_2d) We could separate to a package KomaMRIPhantoms.jl for examples. as they could be quite heavy specialy if we want to add phantoms with realistic data. They also should be stored in the HDF5 phantom file Motion Phantom #183.
  • Do we want the MRD export to be part of the core? The default could always be a matrix and as a package extension when importing using MRIFiles automatically export to MRD.

We need to reevaluate the dependancies given the changes that are being done to the internal interpolation.

@cncastillo cncastillo changed the title Separate simulation base to KomaMRIBase subpackage Separate KomaMRI into simpler subpackages Nov 17, 2023
@beorostica beorostica assigned beorostica and unassigned cncastillo Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) high priority enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants