Refactor AcqInfo and Trajectory Calculation #560
Conversation
ghstack-source-id: a4a2be048199eeaa9cf2647606a6e24fd4a4b307 ghstack-comment-id: 2501006700 Pull Request resolved: #560
ghstack-source-id: 8a5af9ccc79767b8873ef067dcc746bcc6e48f69 ghstack-comment-id: 2501006700 Pull Request resolved: #560
ghstack-source-id: d1ac51bfb1f9c81a7bc7cdbcaf4c0f4e95de0959 ghstack-comment-id: 2501006700 Pull Request resolved: #560
fzimmermann89
changed the title
fzimmermann89
changed the base branch from
gh/fzimmermann89/32/head
to
gh/fzimmermann89/34/head
ghstack-source-id: 1004c06750b10a12ef0e92e22463e872227b5b15 ghstack-comment-id: 2501006700 Pull Request resolved: #560
📚 Documentation |
src/mrpro/data/traj_calculators/KTrajectorySunflowerGoldenRpe.py
Outdated
Show resolved
Hide resolved
src/mrpro/data/AcqInfo.py
Outdated
| discard_post=tensor_2d(headers['discard_post']), | ||
| discard_pre=tensor_2d(headers['discard_pre']), | ||
| encoding_space_ref=tensor_2d(headers['encoding_space_ref']), | ||
| acquisition_time_stamp=tensor_2d(headers['acquisition_time_stamp']).double(), |
There was a problem hiding this comment.
I don't think casting to double is a good idea here and for the PhysiologyTimestamps, because these parameters are very likely to be used in signal models leading than to the whole signal model being evaluated in double precision. I would probably leave it as int - this might be another indicator for users that these are not times in [s] but discrete timesteps
There was a problem hiding this comment.
we should come up with a solution here.
it it is int, it is quite easy to make the mistake "acquisition_time_stamp*2.5" which will not work as expected..
also, if i am not mistaken, we would anyways assume it is in 2.5ms steps when we use it in signal models.
so maybe we can just convert it when loading from ismrmrd to seconds, assuming 2.5ms steps.
if there ever is another source for ismrmrd files that uses a different step, we should handle that case separately.
So i would change the documentation to
"usually in seconds since midnight (Siemens). For other vendors, this will be the vendor time stamp times 2.5e-3"
There was a problem hiding this comment.
I think seconds makes most sense but we should check the vendor and print a warning if not Siemens. Soon we will hopefully have similar data from the low-field scanner and it would be nice to have a higher resolution than 2.5ms...
src/mrpro/data/traj_calculators/KTrajectorySunflowerGoldenRpe.py
Outdated
Show resolved
Hide resolved
| ismrmrd_header.acquisitionSystemInformation is None | ||
| or ismrmrd_header.acquisitionSystemInformation.systemVendor is None | ||
| ): | ||
| warnings.warn('No vendor information found. Assuming Siemens time stamp format.', stacklevel=1) |
There was a problem hiding this comment.
I think here you are missing another convert_time_stamp = convert_time_stamp_siemens # 2.5ms time steps but I would actually suggest to add another converter which interprets the integer time stamps as time in ms and use that if there is no vendor information or if the vendor is unknown.
| note = 'discard_pre and post were applied.' | ||
| else: | ||
| note = 'discard_pre and discard_post were empty.' | ||
| warnings.warn( |
There was a problem hiding this comment.
Should this be moved out of the else statement?
| reversed_readout_mask: torch.Tensor | None = None, | ||
| **_, | ||
| ) -> KTrajectory: | ||
| """Calculate radial phase encoding trajectory for given KHeader. |
There was a problem hiding this comment.
| """Calculate radial phase encoding trajectory for given KHeader. | |
| """Calculate radial phase encoding trajectory for given header information. |
| k2_idx: torch.Tensor, | ||
| reversed_readout_mask: torch.Tensor | None = None, | ||
| **_, | ||
| ) -> KTrajectory: | ||
| """Calculate radial phase encoding trajectory for given KHeader. |
There was a problem hiding this comment.
| """Calculate radial phase encoding trajectory for given KHeader. | |
| """Calculate radial phase encoding trajectory for given header information. |
| k1_idx: torch.Tensor, | ||
| reversed_readout_mask: torch.Tensor | None = None, | ||
| **_, | ||
| ) -> KTrajectory: | ||
| """Calculate radial 2D trajectory for given KHeader. |
There was a problem hiding this comment.
| """Calculate radial 2D trajectory for given KHeader. | |
| """Calculate radial 2D trajectory for given header information. |
| k2_idx | ||
| indices of k2 | ||
| reversed_readout_mask | ||
| boolean tensor indicating reversed readout | ||
|
|
||
| Returns | ||
| ------- | ||
| radial phase encoding trajectory for given KHeader |
There was a problem hiding this comment.
| radial phase encoding trajectory for given KHeader | |
| radial phase encoding trajectory for given header information |
|
The doc string here is IMO also wrong/outdated: |
Removes fields from AcqInfo only requiered for trajectory calculation.
Trajecotry calculators no longer get the full header as parameters.
TODO: Docstrings could be improved