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

Phantom documentation #495

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Phantom documentation #495

wants to merge 8 commits into from

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/

@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.

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