MagneticResonanceImaging / MRIReco.jl

Julia Package for MRI Reconstruction
https://magneticresonanceimaging.github.io/MRIReco.jl/latest/
Other
85 stars 22 forks source link

Draft PR in order to use different number of profiles between contrast / repetition #139

Closed aTrotier closed 1 year ago

aTrotier commented 1 year ago

linked to issue : #138

codecov-commenter commented 1 year ago

Codecov Report

Base: 63.05% // Head: 63.05% // No change to project coverage :thumbsup:

Coverage data is based on head (937dffe) compared to base (00fccd7). Patch has no changes to coverable lines.

:exclamation: Current head 937dffe differs from pull request most recent head f1cfd91. Consider uploading reports for the commit f1cfd91 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #139 +/- ## ======================================= Coverage 63.05% 63.05% ======================================= Files 8 8 Lines 249 249 ======================================= Hits 157 157 Misses 92 92 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

aTrotier commented 1 year ago

@tknopp it was easier than I thought :)

Right now I only need one temporal dimension with dedicated trajectories. But If at some points if I want to implement XD-GRASP reconstruction for cardiac / respiratory, I will also need a second dimension.

Is that ok if I also implement the possibility to have various trajectory along either contrasts and repetitions ? Or do you prefer to let the repetition dimension as it is and later adding another dimension ? (like segment / phase / set if we want to stick to the MRD flags)

tknopp commented 1 year ago

Is that ok if I also implement the possibility to have various trajectory along either contrasts and repetitions ?

Yes sounds good. You can ping me once this is ready to merge.

aTrotier commented 1 year ago

As suggested by @JeffFessler, I have created an example with literate to show how to use that possibility. It works locally for me and gives this html pages : index.html.zip

Maybe @mrikasper also want to take a look for documentation enhancements

aTrotier commented 1 year ago

@tknopp maybe we can merge this PR.

I'll create another PR for the repetition later. It will require a lot of changes and maybe breaking ones.

aTrotier commented 1 year ago

The example is generated with Literate.jl but I don't see the plot. I also had that issue in local because I had to dev the packages. I think the package used in docs/ are not the one on main.

@JeffFessler Do you know how to show the error in the page build by literate ?

JeffFessler commented 1 year ago

I don't have any experience with using Literate when there are subpackages. Seems tricky! For a single-package repo, I first debug the Literate examples locally by running julia -- make.jl in the docs directory like here: https://github.com/JeffFessler/MIRT.jl/tree/main/docs When I think it works, I submit a PR and the CI runs the docs in the cloud and I can look at the action outputs to see if it runs. Especially the "BuildAndDeploy" tab for the Documentation build, such as this: https://github.com/JeffFessler/MIRT.jl/actions/runs/3644407653/jobs/6153624826 You can also use "push preview" to see if it builds properly but I don't use that too often: https://documenter.juliadocs.org/stable/man/hosting/#gh-pages-Branch