Skip to content

Phantom documentation #495

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

Merged
merged 46 commits into from
Jun 16, 2025
Merged

Phantom documentation #495

merged 46 commits into from
Jun 16, 2025

Conversation

pvillacorta
Copy link
Collaborator

@pvillacorta pvillacorta commented Oct 9, 2024

This PR aims to solve #395 and probably #394.

I am using Literate.jl to make all the new documents

https://juliahealth.org/KomaMRI.jl/previews/PR495/

To Do List: #495 (comment)

PD: it would be nice to merge #561 before this PR is merged.

@pvillacorta pvillacorta added the documentation Improvements to docs., it also triggers doc preview label Oct 9, 2024
@cncastillo
Copy link
Member

Nice thanks!

Btw, Julia 1 changed yesterday to the new 1.11 version. Some minor modifications may be needed.

@cncastillo
Copy link
Member

cncastillo commented Oct 9, 2024

Also, unrelated, but I have seen this error occur more than once for the motion tests on Julia 1.10 (Buildkite CI job: https://buildkite.com/julialang/komamri-dot-jl/builds/1209#01927136-fd93-423f-9b2c-f55de2bc3048):

KomaMRICore.BlochSimple: Error During Test at /var/lib/buildkite-agent/builds/sagittarius-maleadt-net/julialang/komamri-dot-jl/KomaMRICore/test/runtests.jl:454
  Got exception outside of a @test
  ArgumentError: Attempt to copy a freed reference.
  Stacktrace:
    [1] copy(ref::GPUArrays.DataRef{oneAPI.oneL0.DeviceBuffer})
      @ GPUArrays ~/.cache/julia-buildkite-plugin/depots/018dadfb-f4d5-466e-8c3f-55b5f9a75025/packages/GPUArrays/qt4ax/src/host/abstractarray.jl:77
    [2] _
      @ ~/.cache/julia-buildkite-plugin/depots/018dadfb-f4d5-466e-8c3f-55b5f9a75025/packages/oneAPI/gsDml/src/array.jl:78 [inlined]
    [3] oneArray
      @ ~/.cache/julia-buildkite-plugin/depots/018dadfb-f4d5-466e-8c3f-55b5f9a75025/packages/oneAPI/gsDml/src/array.jl:71 [inlined]
    [4] derive
      @ ~/.cache/julia-buildkite-plugin/depots/018dadfb-f4d5-466e-8c3f-55b5f9a75025/packages/oneAPI/gsDml/src/array.jl:451 [inlined]
    [5] unsafe_contiguous_view
      @ ~/.cache/julia-buildkite-plugin/depots/018dadfb-f4d5-466e-8c3f-55b5f9a75025/packages/GPUArrays/qt4ax/src/host/base.jl:319 [inlined]
    [6] unsafe_view
      @ ~/.cache/julia-buildkite-plugin/depots/018dadfb-f4d5-466e-8c3f-55b5f9a75025/packages/GPUArrays/qt4ax/src/host/base.jl:314 [inlined]
    [7] view
      @ ~/.cache/julia-buildkite-plugin/depots/018dadfb-f4d5-466e-8c3f-55b5f9a75025/packages/GPUArrays/qt4ax/src/host/base.jl:310 [inlined]
    [8] get_spin_coords(ml::KomaMRIBase.MotionList{Float32}, x::oneAPI.oneArray{Float32, 1, oneAPI.oneL0.DeviceBuffer}, y::oneAPI.oneArray{Float32, 1, oneAPI.oneL0.DeviceBuffer}, z::oneAPI.oneArray{Float32, 1, oneAPI.oneL0.DeviceBuffer}, t::oneAPI.oneArray{Float32, 1, oneAPI.oneL0.DeviceBuffer})

Not sure if it always fails with BlochSimple, but it seems related to a view inside get_spin_coords.

@cncastillo
Copy link
Member

Also, this

julia> using KomaMRI

julia> rot = Rotate(0.0, 0.0, π/2)
Rotate{Float64}
  pitch: Float64 0.0
  roll: Float64 0.0
  yaw: Float64 1.5707963267948966

julia> motion = MotionList(rot)

crashes Julia instead of throwing an error (?)

@pvillacorta
Copy link
Collaborator Author

pvillacorta commented Oct 9, 2024

Btw, Julia 1 changed yesterday to the new 1.11 version. Some minor modifications may be needed.

Buildkite CI for julia v1 (1.11) now fails with oneAPI. I opened this issue, since it looks like a bug.

Also, unrelated, but I have seen this error occur more than once for the motion tests on Julia 1.10 (Buildkite CI job: https://buildkite.com/julialang/komamri-dot-jl/builds/1209#01927136-fd93-423f-9b2c-f55de2bc3048):

Regarding this, I will need to investigate more

For the moment, I commit some changes in order to solve this bug (and also add some useful constructors):

Also, this

julia> using KomaMRI

julia> rot = Rotate(0.0, 0.0, π/2)
Rotate{Float64}
  pitch: Float64 0.0
  roll: Float64 0.0
  yaw: Float64 1.5707963267948966

julia> motion = MotionList(rot)

crashes Julia instead of throwing an error (?)

I'm also trying to pass the docs CI

- Solve MotionList(rot) bug
- Pass Docs CI
- Add useful constructors to `Motion`
@cncastillo
Copy link
Member

cncastillo commented Oct 9, 2024

The docs are failing in Julia 1.11 due to this:

There's already a PR fixing it:

- Finish lit-1-phantom.jl
- New lit-3-phantom-format.jl
- Upload svg files for phantom file tree
- Update cross references
- Typo in lit-1-phantom.jl
- Typo in ph-timespan-type svg filenames
- Fix light mode for ph-timespan-type
- Change phantom-format file to pure markdown
- Try `execute=true` option for Literate.markdown function
- Change `plot_phantom_map` example to display T1
- Remove buildkite test to avoid overloading
Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 86.89%. Comparing base (00a8d8a) to head (022766e).

Files with missing lines Patch % Lines
KomaMRIBase/src/motion/motionlist/Motion.jl 0.00% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
- Coverage   90.77%   86.89%   -3.88%     
==========================================
  Files          54       54              
  Lines        3002     3007       +5     
==========================================
- Hits         2725     2613     -112     
- Misses        277      394     +117     
Flag Coverage Δ
base 86.81% <10.00%> (-0.26%) ⬇️
core 72.95% <ø> (-20.25%) ⬇️
files 93.28% <ø> (+1.58%) ⬆️
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/motion/motionlist/MotionList.jl 67.64% <100.00%> (ø)
KomaMRIPlots/src/ui/DisplayFunctions.jl 94.17% <ø> (ø)
KomaMRIBase/src/motion/motionlist/Motion.jl 58.62% <0.00%> (-26.38%) ⬇️

... and 14 files with indirect coverage changes

- Remove `execute=true`
- Update svg files for file format section
- Display outputs in phantom section
@pvillacorta
Copy link
Collaborator Author

pvillacorta commented Oct 11, 2024

I still have to:

  • Complete the file format section
  • Write the motion section
  • Tests of the new Motion methods in order to pass CI
  • Improve The how-to section. Maybe I should just add a sub-section explaining how to import/export phantom files
  • Solve this (402439b):

Another minor bug I found:

obj = Phantom(x=zeros(100), z=collect(range(-5e-2, 5e-2, 100)))
plot_pantom_map(obj, :T1)

This plots incorrectly.

The docs should be built correctly now.

@cncastillo
Copy link
Member

cncastillo commented Oct 15, 2024

Another minor bug I found:

obj = Phantom(x=zeros(100), z=collect(range(-5e-2, 5e-2, 100)))
plot_pantom_map(obj, :T1)

This plots incorrectly.

The docs should be built correctly now.

@cncastillo
Copy link
Member

Are there any fixes in this branch? Maybe we can move them to the other one to tag a new patch release without waiting for the documentation.

@pvillacorta pvillacorta marked this pull request as ready for review April 10, 2025 10:43
@pvillacorta
Copy link
Collaborator Author

I added an extra tutorial for simulating cardiac arrhythmia (variable RR intervals).
This should be ready to merge, but maybe after #561.

Copy link
Member

@cncastillo cncastillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is looking great, just a few minor comments. Is there a way to show how the phantom moves after the arrhythmias are introduced?

@pvillacorta
Copy link
Collaborator Author

Is there a way to show how the phantom moves after the arrhythmias are introduced?

Yes! Once the variable RR intervals are introduced, just plot the phantom again:
12 06 2025_21 15 04_REC

I can plot it again in the tutorial, although for me it looks a bit repetitive. What do you think?

@cncastillo
Copy link
Member

I think it is fine to not include it, you cant see the arrhythmia very well, maybe with a finer temporal resolution ... but don't worry about it.

- `execute=true` in `Literate.markdown` function
- Add Suppressor and DisplayAs packages
- Adapt all examples to `execute=true` option
@cncastillo cncastillo merged commit dc91593 into master Jun 16, 2025
7 of 13 checks passed
@cncastillo cncastillo deleted the docs-phantom branch June 16, 2025 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements to docs., it also triggers doc preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Phantom Explanation section Improve "How to Create your Own Phantom" section
2 participants