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

Fix Phantom-related bugs (#498 and more) #499

Merged
merged 8 commits into from
Nov 3, 2024
Merged

Conversation

pvillacorta
Copy link
Collaborator

@pvillacorta pvillacorta commented Oct 17, 2024

This PR solves the following:

  • Troubles defining Float32 or empty phantoms with KomaMRI 0.9 #498
    Phantom(x=[]), Phantom{Float64}(x=[]) or Phantom{Float32}(x=[]) are no longer supported*, since the element type of the Phantom is Inferred from its input arguments, and defining x=[] (eltype Any) is not a good practice in Julia.
    Now, it is possible to define an empty Phantom this way:

    Phantom() # default eltype is Float64
    Phantom{Float64}()
    Phantom{Float32}()
    Phantom{Int64}()
    (...)

    *The example in this page is updated too.

  • This bug when creating a motionlist from an action. I have also added some Motion constructors that might be useful.

  • plot_phantom_map: now 2D or 1D phantoms in yz plane are displayed correctly. See this comment. Moreover, the prefix for the time slider in dynamic plots was incorrect ("x="). This has changed to be a ("t=").

@pvillacorta
Copy link
Collaborator Author

Buildkite CI is still failing because of JuliaGPU/oneAPI.jl#473

@cncastillo
Copy link
Member

Is there any other problem besides oneAPI.jl not working on Julia 1.11? We could merge this and register a new version.

oneAPI.jl, at some point, will include fixes for the logical indexing problem, as there has been movement with using KernelAbstractions.jl as the backend for GPUArrays.jl.

@pvillacorta
Copy link
Collaborator Author

I added the commits from #495 that fixed bugs. It should be ready to merge

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.

This PR has too many changes, based on the title I would have expected it to only change KomaMRIBase. Please separate the other changes, to register this new fix in komamribase, without the rest.

KomaMRIBase/src/motion/motionlist/Motion.jl Outdated Show resolved Hide resolved
KomaMRIPlots/src/ui/DisplayFunctions.jl Outdated Show resolved Hide resolved
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 this PR also changes the name of a folder, which confused the diif's. please maintain the the name of the folders and files.

Also verify if all the docs changes are correct.

KomaMRIBase/src/motion/MotionList.jl Outdated Show resolved Hide resolved
@pvillacorta
Copy link
Collaborator Author

I think it is ready now

@cncastillo
Copy link
Member

cncastillo commented Nov 1, 2024

I think the plotting changes came back (fixing another bug).

@pvillacorta
Copy link
Collaborator Author

I think the plotting changes came back (fixing another bug).

Oh, I didn't get to remove those changes at any point in the end. Don't you think it's a good idea to take the opportunity to fix those bugs as well?

@cncastillo
Copy link
Member

Not the best practice, but let's change the title of the PR and list the bugs this PR is solving. So at least someone looking at it can understand.

@pvillacorta pvillacorta changed the title Fix bug when creating an empty Phantom Fix Phantom-related bugs (#498 and more) Nov 2, 2024
@cncastillo cncastillo merged commit 17182dd into master Nov 3, 2024
10 of 12 checks passed
@cncastillo cncastillo deleted the fix-empty-phantom branch November 3, 2024 14:28
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.

Troubles defining Float32 or empty phantoms with KomaMRI 0.9
2 participants