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 and enhanced documentation #211

Open
1 of 13 tasks
aTrotier opened this issue Dec 11, 2024 · 7 comments
Open
1 of 13 tasks

Fix and enhanced documentation #211

aTrotier opened this issue Dec 11, 2024 · 7 comments
Labels
documentation Documentation needed good first issue Good for newcomers

Comments

@aTrotier
Copy link
Contributor

aTrotier commented Dec 11, 2024

To fix :

To add :

Should we try to run most of the example during the CI and perform a doctest if it fails ?

@aTrotier aTrotier added good first issue Good for newcomers documentation Documentation needed labels Dec 11, 2024
@cncastillo
Copy link
Contributor

Regarding the interop with KomaMRI. @Stockless is currently working on incorporating parallel imaging (https://github.com/orgs/MagneticResonanceImaging/discussions/208), so we will definitely have examples of "simulationg with Koma" -> "Estimate coils sens from raw data (BARTIO/MRICoilSensitivities)" -> "Reconstruct with MRIReco" in our docs, but we are happy to put something in MRIReco as well. We could contribute a Literate or Pluto notebook example.

Now that I saw flags in the list I remembered something. I believe that the flags need to be explicitly defined as UInt64 to be compatible with Python's ISMRMRD reader, otherwise they are read incorrectly. That is why I defined them like this in Koma:

https://github.com/JuliaHealth/KomaMRI.jl/blob/master/KomaMRICore/src/rawdata/ISMRMRD.jl#L1-L44

We might want to include this in MRIBase?

@aTrotier
Copy link
Contributor Author

For flags, I have implemented them in uint64 no ?

https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/master/MRIBase/src/Datatypes/Flags.jl
and the corresponding header :

@cncastillo
Copy link
Contributor

Yes, but I think the problem is that the flags do not necessarily correspond to the required UInt64 mask if you do it like that (if I remember correctly).

# "My method"
julia> const b64 = UInt64(1)
0x0000000000000001

julia> const ISMRMRD_ACQ_USER8 = 1b64 << ( 64 - 1 )
0x8000000000000000

# MRIReco
julia> flag::UInt64 = 64 # ACQ_USER8 in https://magneticresonanceimaging.github.io/MRIReco.jl/latest/acquisitionData/#flags
64

julia> flag
0x0000000000000040

julia> flag == ISMRMRD_ACQ_USER8
false

Let me quickly check.

@aTrotier
Copy link
Contributor Author

The interface for flag is supposed to be with set_flags!()

head = AcquisitionHeader()
traj = Matrix{Float32}(undef,0,0)
dat =  Matrix{ComplexF32}(undef,0,0)
p = Profile(head,traj,dat)

const b64 = UInt64(1)
const ISMRMRD_ACQ_USER8 = 1b64 << ( 64 - 1 )
MRIBase.flag_set!(p,64)

p.head.flags == ISMRMRD_ACQ_USER8

Returns true. I don't know if we can force the user to not mess directly with the flags : p.head.flags=64

@cncastillo
Copy link
Contributor

cncastillo commented Dec 12, 2024

Oh ok, didn't know MRIBase.flag_set! existed. That function already takes care of the bit mask.

My suggestion was to predefine the flags as the bit mask (mask = bitshift(UInt64(1), b - 1)) so we can do p.head.flags = ISMRMRD_ACQ_USER8 | ISMRMRD_ACQ_USER7 or p.head.flags = FLAGS["ACQ_USER8"] | FLAGS["ACQ_USER7"] which I argue is more intuitive.

Edit: I guess this is copying how they do it in the ISMRMRD packages, at least a different FLAG_MASKS could be included to be able to modify flags directly.

@aTrotier
Copy link
Contributor Author

It is something I have done last week.
Yes I have replicated the implementation from ISMRMRD-matlab / python in order to get something harmonised between multiple implementation.

I can make the modification before we perform a release. Do you have a preferences between p.head.flags = ISMRMRD_ACQ_USER8 | ISMRMRD_ACQ_USER7 or p.head.flags = FLAGS["ACQ_USER8"] | FLAGS["ACQ_USER7"] @tknopp @nHackel

In ismrmrd the implementation is closer to p.head.flags = FLAGS["ACQ_USER8"] | FLAGS["ACQ_USER7"] https://github.com/ismrmrd/ismrmrd/blob/master/matlab/%2Bismrmrd/AcquisitionHeader.m

and in gadgetron-matlab p.head.flags = ISMRMRD_ACQ_USER8 | ISMRMRD_ACQ_USER7 https://github.com/gadgetron/gadgetron-matlab/blob/fbc84e5659e6bca36fbd0348ea46d1facc8670ba/%2Bgadgetron/%2Btypes/Acquisition.m#L68

gadgetron.jl users flagsets.jl ... https://github.com/gadgetron/Gadgetron.jl/blob/main/src/Acquisition.jl

and ismrmrd-python : https://github.com/ismrmrd/ismrmrd-python/blob/b93bc1bf351bfd949a73343da3547be58922b819/ismrmrd/constants.py#L21

@nHackel
Copy link
Member

nHackel commented Dec 13, 2024

We could keep both interfaces and add some type checking/sanity checks. Or we can just do the sanity checks.

Since we defined the head type we can define our own setters for it. head.flags = ... is syntactic sugar for setproperty!(head, :flags, ...) which per default calls setfield!(head, :flags, ...). This level of abstraction allows us to add some sanity checks for the flags:

function setproperty!(head::AcquisitionHeader, name::Symbol, value)
  if name == :flags
    throw(TypeError("Flags can only be directly set with UInt64 or with `flag_set!", UInt64, value))
  else
    setfield!(head, name, convert(fieldtype(head, name), value))
  end
end
setproperty!(head::AcquisitionHeader, name::Symbol, value::UInt64) = setfield!(head, name, convert(fieldtype(head, name), value))

I don't have a strong preference between ISMRMRD_ACQ_USER8 and FLAGS["ACQ_USER7"]. The first option might have more help from auto-complete and a user can't change the const values, while a user could in theory overwrite an entry in the dict.

If we go with both interfaces, we could also add a utility function for flag_set! and co. which accepts a vector or vararg of "normal" integers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation needed good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants