Conversation
There was a problem hiding this comment.
CASL is still not overwritten by PCASL, perhaps you forgot to commit that change?
Otherwise, I'm happy. Let me know when I can tell the Hong Kong guys that they can use the develop branch.
@jan-petr I don't know what you're talking about. xASL_bids_BIDSifyASLJSON section 5 clearly prioritizes DICOM fields over studyPar fields, and ArterialSpinLabelingType is not defined as exception.
BTW: Field name conversions occur after, shouldn't they occur before comparing the DICOM and StudyPar fields?
|
Now we understand one another :) {"Modality":"MR", |
|
If I compare -"ScanningSequence":"RM\IR", |
|
Bug identified - when merging 2 parts of NII in DICOM2NII, we merge also JSONs, but without prioritizing ASL part over M0 - this has to change as the M0 part has errors in it. |
| return; | ||
| end | ||
|
|
||
| % Set priorities for merging JSONs, higher number = higher priority |
There was a problem hiding this comment.
This is a bit tricky perhaps.
- Note that in the case of GE, it is a deltaM image, not CBF (which it spits out separately). So deltaM should probably go before m0scan. And control/label as well?
- I don't think we should generalize this. For BIDS, we want to keep NIfTIs as how they were acquired. GE/dcm2niiX just has a bug by splitting their single sequence here. Siemens as well I guess. But the generalized solution is to keep the output as it was.
There was a problem hiding this comment.
-
deltaM HAS the highest priority - higher number has the highest priority (as said in the comment in the code). so deltaM overwrites all. deltaM > control/label > m0scan > cbf (this is there just for completeness). I don't understand your comment, deltaM already goes before anything else.
-
Indeed, we are not generalizing. All this priority stuff is done in a GE subfunction only applying for GE. For Siemens and Philips - JSONs are merged with equal priorities.
There was a problem hiding this comment.
- Ah I see now. Then I would just mention them in the order such that 1 and 0 are together, now this is confusing to read. So keep the code as is, just 4 3 2 1 0 instead of 1 2 3 4. If that's done, you can resolve this comment.
- OK, this prioritization of merging could actually be general. Also, since you reuse it in xASL_bids..._Merge, this sounds generic. But fine for now how it is.
There was a problem hiding this comment.
I've clarified this in the comment. I don't agree to merge 0 and 1 because a known file type (CBF) should have a higher priority than unknown (that has a zero priority).
Changing the order of code doesn't make any sense - it won't affect the readability at all. Sorry, but not doing things that don't make any sense ;) The options are listed in order of increasing priority. Otherwise is always listed at the end.
Just as an FYI: The prioritization function is general. But the setting of priorities is only used in specific cases when we know.
|
|
||
| % Go through all fields | ||
| listFieldNames = fieldnames(currentJSON); | ||
| for iFieldName = 1:length(listFieldNames) |
There was a problem hiding this comment.
Is this field-wise compiling of the merged JSON the best solution? Why doing this instead of just prioritizing one full JSON that dcm2nii outputted?
(not saying it's not a good idea, just saying that this may or may not work. One disadvantage I can think of, is that you potentially get a mix of values belonging to a label and an M0 image).
There was a problem hiding this comment.
I'm not really sure either - previously, I was always taking a random JSON, because there were no differences between them. This is the first case encountered that they actually differ :)
So hard to say if it is better to select one or compile all :) Or even impossible to say - both can be later proven to be wrong :) For now, I would slightly prefer merging (and checking if there's nothing extra), then selecting one and potentially missing something.
But if you think that selecting is better, I am not against ;)
There was a problem hiding this comment.
I would at least issue warnings if there are differences between the JSONs. Then I would choose the priority JSON. Then you can resolve :)
There was a problem hiding this comment.
I've added that for LabDur and PLD - and we can add more important fields. But otherwise this makes no sense. There are a lot of differences between JSONs in parameters not important for ASL. It would be issuing to many warnings if warning for all.
HenkMutsaerts
left a comment
HenkMutsaerts
left a comment
There was a problem hiding this comment.
See last changes, if you processed them, I'm fine and don't have to re-review :)
| return; | ||
| end | ||
|
|
||
| % Set priorities for merging JSONs, higher number = higher priority |
There was a problem hiding this comment.
- Ah I see now. Then I would just mention them in the order such that 1 and 0 are together, now this is confusing to read. So keep the code as is, just 4 3 2 1 0 instead of 1 2 3 4. If that's done, you can resolve this comment.
- OK, this prioritization of merging could actually be general. Also, since you reuse it in xASL_bids..._Merge, this sounds generic. But fine for now how it is.
|
|
||
| % Go through all fields | ||
| listFieldNames = fieldnames(currentJSON); | ||
| for iFieldName = 1:length(listFieldNames) |
There was a problem hiding this comment.
I would at least issue warnings if there are differences between the JSONs. Then I would choose the priority JSON. Then you can resolve :)
|
The latest commit messed up things. Trying to revert it. {'GE_PCASL_3Dspiral_DV22.0_1' } {'rawdata' } {'Missing file' } {'/sub-Sub1/perf/sub-Sub1_asl.json' } |
2 participants
Linked issue
Closes #1201