Fixes #762 xASL_stat_GetDICOMStatistics: Improve TSV cell array fixing #763

MichaelStritt · 2021-08-04T12:53:52Z

Linked issue

#762

Comments

@BeatrizPadrela & @HenkMutsaerts: would be cool if you would test this to validate that it's working correctly.

HenkMutsaerts

Would resort to 'n/a', otherwise good!

Modules/SubModule_Population/xASL_stat_GetDICOMStatistics.m

HenkMutsaerts · 2021-08-04T14:01:22Z

Modules/SubModule_Population/xASL_stat_GetDICOMStatistics.m

+    for iColumn=1:size(TSV,2)
+        % Remove lists (we had a problem where it was tried to insert SliceTiming arrays into the table)
+        if size(TSV{iRow,iColumn},1)>1
+            TSV{iRow,iColumn} = 'n/a';


But shouldn't this produce a warning? The pipeline should never suddenly introduce lists inside one cell. This could be the case in the import module, but not in the other modules.

They could be defined manually though. I only encountered this once, but for multiple visits/subjects you could potentially get a LOT of warnings

Sure, then we can keep track of the warning and limit the number of its outputs

I added a warning for now, let's see how it turns out fd9f2f4

I have an alternative solution for more transparency. If there's a list, then report it is a list instead of "n/a". You can either print "list" or "field1, field2,..."

HenkMutsaerts · 2021-08-04T14:02:16Z

Modules/SubModule_Population/xASL_stat_GetDICOMStatistics.m

+        end
+        % Remove empty elements (Empty cells in the TSV can lead to reading errors)
+        if isempty(TSV{iRow,iColumn})
+            TSV{iRow,iColumn} = '_';


Why the underscore? Why not 'n/a' like in the other cases?

Because we write this table where we have empty places and if we write '' to the cell array some tsv readers crash. We did this before and it looks quite okay I think.

We have things like:
row description COLUMN 1 COLUMN2 COLUMN 3
row description value value value
row description EMPTYCOLUMN COLUMN 1 COLUMN2
row description _ value value

I don't like it, but for that use case it's the cleanest solution IMHO

I like this strategy, I just think that '_' is arbitrary, whereas BIDS defaults to 'n/a' for missing values. So it seems we should use this as well right?

I introduced the '_' only for these placeholder cells that you used in these combined tables. All the numerical values should be defaulted to 'n/a'. Check out the example below:

I am fine with that. I would merge as is and change in the future if we find this weird. Unless Henk has a different opinion.

Ah I see, nice. So all missing values, irrespective their meaning/format, will be 'n/a', according to BIDS. Only the empty placeholder cells will be an underscore '_', that looks good.

HenkMutsaerts · 2021-08-04T14:21:46Z

Modules/SubModule_Population/xASL_stat_GetDICOMStatistics.m

+    for iColumn=1:size(TSV,2)
+        % Remove lists (we had a problem where it was tried to insert SliceTiming arrays into the table)
+        if size(TSV{iRow,iColumn},1)>1
+            TSV{iRow,iColumn} = 'n/a';


Sure, then we can keep track of the warning and limit the number of its outputs

Modules/SubModule_Population/xASL_stat_GetDICOMStatistics.m

MichaelStritt · 2021-08-16T15:17:04Z

@jan-petr could you check this out while Henk is on vacation?

jan-petr

See my comments. Let's have a final consensus with Henk. But in principle, we can merge.

jan-petr · 2021-08-16T17:18:34Z

Modules/SubModule_Population/xASL_stat_GetDICOMStatistics.m

+    for iColumn=1:size(TSV,2)
+        % Remove lists (we had a problem where it was tried to insert SliceTiming arrays into the table)
+        if size(TSV{iRow,iColumn},1)>1
+            TSV{iRow,iColumn} = 'n/a';


I have an alternative solution for more transparency. If there's a list, then report it is a list instead of "n/a". You can either print "list" or "field1, field2,..."

jan-petr · 2021-08-16T17:19:59Z

Modules/SubModule_Population/xASL_stat_GetDICOMStatistics.m

+        end
+        % Remove empty elements (Empty cells in the TSV can lead to reading errors)
+        if isempty(TSV{iRow,iColumn})
+            TSV{iRow,iColumn} = '_';


I am fine with that. I would merge as is and change in the future if we find this weird. Unless Henk has a different opinion.

HenkMutsaerts · 2021-08-17T11:07:38Z

Modules/SubModule_Population/xASL_stat_GetDICOMStatistics.m

+        end
+        % Remove empty elements (Empty cells in the TSV can lead to reading errors)
+        if isempty(TSV{iRow,iColumn})
+            TSV{iRow,iColumn} = '_';


Ah I see, nice. So all missing values, irrespective their meaning/format, will be 'n/a', according to BIDS. Only the empty placeholder cells will be an underscore '_', that looks good.

MichaelStritt requested a review from HenkMutsaerts August 4, 2021 12:54

MichaelStritt assigned BeatrizPadrela Aug 4, 2021

MichaelStritt added the bug Something isn't working label Aug 4, 2021

MichaelStritt linked an issue Aug 4, 2021 that may be closed by this pull request

xASL_stat_GetDICOMStatistics: bug with arrays in cell arrays #762

Closed

HenkMutsaerts requested changes Aug 4, 2021

View reviewed changes

HenkMutsaerts assigned MichaelStritt and unassigned BeatrizPadrela Aug 4, 2021

MichaelStritt requested a review from HenkMutsaerts August 4, 2021 14:11

MichaelStritt assigned BeatrizPadrela and unassigned MichaelStritt Aug 4, 2021

HenkMutsaerts requested changes Aug 4, 2021

View reviewed changes

HenkMutsaerts assigned MichaelStritt and unassigned BeatrizPadrela Aug 4, 2021

MichaelStritt requested review from HenkMutsaerts and jan-petr August 16, 2021 15:06

jan-petr approved these changes Aug 16, 2021

View reviewed changes

HenkMutsaerts approved these changes Aug 17, 2021

View reviewed changes

MichaelStritt added 2 commits August 17, 2021 14:37

#762 xASL_stat_GetDICOMStatistics: Improve TSV cell array fixing

cd88f57

#762 xASL_stat_GetDICOMStatistics: Add warning and note

add6a58

MichaelStritt force-pushed the bug-#762_FixCellArrays branch from fd9f2f4 to add6a58 Compare August 17, 2021 12:37

MichaelStritt merged commit add6a58 into develop Aug 17, 2021

MichaelStritt deleted the bug-#762_FixCellArrays branch August 17, 2021 12:37

Fixes #762 xASL_stat_GetDICOMStatistics: Improve TSV cell array fixing #763

Fixes #762 xASL_stat_GetDICOMStatistics: Improve TSV cell array fixing #763

Uh oh!

Conversation

MichaelStritt commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked issue

Comments

Uh oh!

HenkMutsaerts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaelStritt commented Aug 16, 2021

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MichaelStritt commented Aug 4, 2021 •

edited

Loading