Repository navigation
Add Twix, MRS and PMU raw data readers - #2
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
==========================================
+ Coverage 86.60% 86.85% +0.25%
==========================================
Files 5 8 +3
Lines 224 312 +88
==========================================
+ Hits 194 271 +77
- Misses 30 41 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tclose
left a comment
There was a problem hiding this comment.
Thanks @jamesdidathing, it looks great! Just have a few minor comments and suggestions on naming conventions
| from fileformats.medimage.dicom import DicomImage | ||
| from fileformats.vendor.siemens.medimage import ( | ||
| SiemensPuls, | ||
| SiemensRda, |
There was a problem hiding this comment.
Re naming convention, the format of the raw data files should be consistent with the console software platform, which for the Cima X is called "Syngo MR XA". FileFormats automatically maps Python class names to "mime-like" string with _ -> . and [A-Z] -> -[a-z] so fileformats.vendor.siemens.medimage.SyngoMr_Xa_Puls -> medimage/vnd.siemens.syngo-mr.xa.puls.
There was a problem hiding this comment.
Didn't realise this, changed them now
There was a problem hiding this comment.
Actually, I should have written SyngoMr_Xa_Puls with the lowercase r to match the convention. Would you mind find/replacing that?
| SyngoMi_Vr20b_Normalisation, | ||
| SyngoMi_Vr20b_Parameterisation, | ||
| SyngoMi_Vr20b_CtSpl, | ||
| TwixRawData, |
There was a problem hiding this comment.
SyngoMr_Xa_Twix is probably a good name
| binary = True | ||
|
|
||
| @validated_property | ||
| def version_is_ve(self) -> bool: |
There was a problem hiding this comment.
The Cima is XA (the next version after VE). Do the twixtools support it?
There was a problem hiding this comment.
So from what I can tell, VE and XA have the same underlying structure so it should handle correctly. It works with the example file you sent over to me, but might be worth testing with some more files?
The method name/docstring I can change too if its confusing.
|
NB: I will create a separate PR to fix the mac-os ci/cd problem |
This is done now. Could you rebase on main? |
twixtools), exposes measurement count, and reads protocol headers via twixtoolsReviewers:
Run tests and ensure they pass correctly, and review the code and logic