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 plot phantom function #413

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Fix plot phantom function #413

merged 5 commits into from
Jun 27, 2024

Conversation

pvillacorta
Copy link
Collaborator

@pvillacorta pvillacorta commented Jun 18, 2024

This PR is intended to fix #407
I also correct some typos in the docstrings for some SimpleMotion types

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (4ad545e) to head (0f4a85b).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #413   +/-   ##
=======================================
  Coverage   89.19%   89.19%           
=======================================
  Files          49       49           
  Lines        2803     2803           
=======================================
  Hits         2500     2500           
  Misses        303      303           
Flag Coverage Δ
base 86.42% <ø> (ø)
core 85.68% <ø> (ø)
files 93.70% <ø> (ø)
komamri 93.98% <ø> (ø)
plots 89.27% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...datatypes/phantom/motion/simplemotion/HeartBeat.jl 92.85% <ø> (ø)
...s/phantom/motion/simplemotion/PeriodicHeartBeat.jl 0.00% <ø> (ø)
...es/phantom/motion/simplemotion/PeriodicRotation.jl 90.47% <ø> (ø)
...phantom/motion/simplemotion/PeriodicTranslation.jl 81.81% <ø> (ø)
.../datatypes/phantom/motion/simplemotion/Rotation.jl 90.47% <ø> (ø)
...tatypes/phantom/motion/simplemotion/Translation.jl 81.81% <ø> (ø)
KomaMRIPlots/src/ui/DisplayFunctions.jl 90.47% <0.00%> (ø)

@cncastillo
Copy link
Member

cncastillo commented Jun 25, 2024

Hi I don't think this solves the problem in #407 yet. I think you need to think about how to use plot_koma properly and redefine it if you need to. The plots should work with no problems for NoMotion and SimpleMotion/ArbitraryMotion for:

  • Julia REPL
  • VScode
  • Pluto
  • Jupyter
  • KomaUI

My understanding is that now, plot_phantom_map with motion has problems with some of those.

The main problem is that the current implementation also breaks plots without motion. For phantoms with NoMotion, the function should do exactly what was done before (like for the simulation), with just a few minor modifications (like the spin subsampling for example). In the meantime, I would only use Plot for the motion plots, and plot_koma for the others (dispatch), sharing some code for the init as functions (no function definitions inside another function).

  • use Plot for the motion plots, and plot_koma for the others (dispatch plot_phantom_map based on Phantom{MotionModel})
  • sharing some code for the init as functions (no function definitions inside another function)

@cncastillo cncastillo merged commit 431c164 into master Jun 27, 2024
18 checks passed
@cncastillo cncastillo deleted the fix-plot-phantom branch June 27, 2024 23:46
@cncastillo cncastillo restored the fix-plot-phantom branch June 28, 2024 00:03
@pvillacorta pvillacorta deleted the fix-plot-phantom branch June 28, 2024 08:22
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.

[KomaUI Plotting] obj_ui[] does not draw on first time updated. Need to press :rho or other option.
2 participants