prevent arraybuffers from being excluded of naturalized datasets #229
Conversation
There was a problem hiding this comment.
Thanks for the contribution 👍
I think the fix makes sense, but would like @wayfarer3130 to have a look.
Two requests:
- Can you expand on the commit message to describe why this was problematic and why your patch fixes the issue. We now have several people editing the code and we need to help each other understand what we are all doing.
- Can you change the commit message to follow semantic-commit format so the patch release is generated automatically. Force push the fork if you need to.
| @@ -149,7 +149,8 @@ class DicomMetaDictionary { | |||
| if ( | |||
| sqZero && | |||
| typeof sqZero === "object" && | |||
| !sqZero.length | |||
| !sqZero.length && | |||
| !(sqZero instanceof ArrayBuffer) | |||
There was a problem hiding this comment.
@wayfarer3130 can you look and test?
There was a problem hiding this comment.
I'm looking at it, but I don't know under what conditions it would be an instance of an array buffer.
Would like to see an example integrated into a unit test.
There was a problem hiding this comment.
That makes sense for Seg, or anything else that decodes direct to raw buffers, still would like to see a unit test.
There was a problem hiding this comment.
addAccessors function is fine, the problem comes from the conditions to which it's being called. Before, it would make naturalDataset[naturalName] = naturalDataset[naturalName][0] and now it leaves it as is which will breaks downstream code and make it undefined.
Have no ideia why it made sense before to remove the element out of the array but code was made with that assumption and the proposed safeguard just tries to make it backward compatible.
anyway, I'll put a test later if it helps
…ors. Since addAccessor's function iterates on object keys only, arraybuffers will be set to undefined causing errors
|
Hi Pieper, Sure, problem arise when loading a dicom-seg files which trigger I believe addAccessor's function is not intended for arrays or arraybuffers so the additional filter makes sense I guess.
|
|
Thanks for the context 👍 I agree a test would help. Another small thing I think there cannot be a space between the |
In the case of segmentation files, PixelData key of naturalized datasets should be an arraybuffer but if added accessors it will be an array with an arraybuffer inside, which breaks further processing added relevant test on test_oneslice_seg of data part
|
Hi! What's the progress on this? I'm currently encountering this very issue in 0.19.6 with PixelData in CT images being placed inside an array after the changes to sequence naturalization. I currently have a workaround for this but naturally I would prefer to see this fix merged to master. |
|
@wayfarer3130 can you comment on this PR to see if it's good to merge? Specifically how does it relate to #251 which seems to touch the same code. |
No description provided.