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

DCFData and CSMData #609

Open
fzimmermann89 opened this issue Jan 16, 2025 · 4 comments
Open

DCFData and CSMData #609

fzimmermann89 opened this issue Jan 16, 2025 · 4 comments

Comments

@fzimmermann89
Copy link
Member

fzimmermann89 commented Jan 16, 2025

We created these data structures with headers and decided to see if it annoys us or if it helps us.
In the last year, nobody has touched the headers and I haven't seen any use.

Usually, one is only interested in the operators.

  1. Should we just drop DCFData and CSMData and only keep the operators (with some classmethods for creating them).
  2. If we keep them, should we remove the header information?
  3. Should CSMData really require an IData to create it? If we remove the header, we could also just create them from an image tensor

We could also make CSMData == SensitvityOp and DCFData==DensityCompensationOp, i.e. making them objects that are both "Data" (concerning indexing, for example) and Operators.

This ineeds to be decided to fix moving reconstruction objects to GPU, which is currently broken,

@ckolbPTB
Copy link
Collaborator

I think DCFData can be removed. The only use case I see is that, if KData is subsetted then we could subset also DCFData rather than to have to recalculate it, but in most cases recalculating it is fine.

CSMData is currently not used at all. The main use case would be to be able to use the coil reference scan (which is always 3D with a pre-defined FOV and resolution) for SENSE reconstructions. For this we would need to reorient the CSMData to the KData based on the header information. First reorienting the IData could be an option but not ideal for 2D slices. Nevertheless, also edge case of the future. Happy to have it removed for the time being.

@fzimmermann89
Copy link
Member Author

Regarding CSMData:

Here I see the most use in indexing, if CSMS come from a different scan than the one we want to reconstruct, for example.

Is the 3D calibration scan data already rotated such that one of the axes is matching the slice axis of the 2D scan? Or is it in a fixed coordinate system? It sound like we would have to do 3D interpolation on it ? ... maybe using GridSampleOp ?
What do we need to have in the header for that edge case? voxel_size and orientation?

A middle ground could be the hybrid CSMData/SensitivityOperator with optional QHeader?
Then, if we ever implement the resample for I/QData as discussed via slack in, we could still use it, and we could use the same indexing logic as we use for KData?

@fzimmermann89
Copy link
Member Author

Mhm... dataclass and torch.module do not work well together.

So, maybe keep CSMData but

  1. make the header optional, thus allowing creation from tensor alone without a header
  2. make CSMData <-> SensititivOp conversion possible, so SensitivityOp.data property returns a CSMData (save the header also in sensitivity op)
  3. add the classmethods also to sensitivityop, that just call the ones in csmdata (or move them and have the ones in csmdata call the operator's)
  4. change examples etc to create sensitivityop instead of csmdata

that way, if weever want to do indexing are resampling, we would do it via
new_op = csm_op.data.resample(...).as_operator()

@ckolbPTB
Copy link
Collaborator

Or is it in a fixed coordinate system? It sound like we would have to do 3D interpolation on it ?

Yes, we would need to have a resample() function because the reference scan is usually in a fixed orientation and independent to the diagnostic scan.

What do we need to have in the header for that edge case? voxel_size and orientation?

Yes and position

new_op = csm_op.data.resample(...).as_operator() sounds like a good idea but I would also be happy with new_op = CsmData(data=csm_op.data, header=idata.header).resample(...).as_operator() if we think having the CsmData.header in the SensitivityOp complicates things.

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

No branches or pull requests

2 participants