Handle unknowns completely #235
Conversation
This change ensures that unknown tags with a defined VL are read as bytes (OW). This should fix suyashkumar#231. Previously they would have been read as strings by default.
…plicit transfer syntax and undefined length
| case "UN": | ||
| return VRUnknown |
There was a problem hiding this comment.
just a note/thought to myself...wondering if we need to revisit the point of having VRKind construct. There's new behavior now, with VRUnknown, the value could actually be represented in two different containers: as a sequence or as bytes (which is new, at least given the behaviors of other items in the list). And anyway, it's not like we need VRKind to unpack the value stored in the container (we could use ValueType for that). So basically the VRKind construct at the moment just serves to "group" some VRs together (e.g. OW and OB) into the type of value we unpack it as in our program. But in reality, we don't need this and could just switch on the VRs directly in readValue or possibly group raw VRs by ValueType we wish to represent them as internally (but again, this doesn't work for UN, because we need other information to determine if we'll store it as bytesValue or sequenceValue).
Anyway, just some quick off the top of my head thoughts for me to revisit later.
|
Oh hey, there's some build failures due to a duplicate switch/case as well. |
|
Huh, I'm just not seeing the duplicate switch/case. I only see the |
|
Hey! Going to close and re-open this to re-run the checks, and start looking into what's up with the build here. |
|
Seems like updating the go version helps with this? See |
Update go version in build
|
For some reasons the checks are not triggering--looking into this more. |
|
Could be the issue that benchstat isn't found in the GitHub actions environment for some reason. Maybe they've changed the way they handle paths with respect to GOBIN. Going to see if fixing that does anything. |
|
Also, I see you're wanting to merge this into s/read-un-as-sq--that seems reasonable, I can merge this into there and then polish it up as needed before merging into main (I'll list you as a co-author of course). I'll take it from here! Thank you for the contribution! |
|
Hi Folks, Thank You! |
This addresses #220 (and one of the dicoms in #327) by treating elements with a VR=UN and undefined VL as Sequences. This pulls from work that I and @jabillings did previously (#235). Some follow on changes may be needed to support this part of [the spec CP](https://dicom.nema.org/dicom/cp/cp246_01.pdf): > If at some point an application knows the actual VR for an Attribute of VR UN (e.g. has its own applicable data dictionary), it can assume that the Value Field of the Attribute is encoded in Little Endian byte ordering with implicit VR encoding, irrespective of the current TransferSyntax. --------- Co-authored-by: jabillings <jabillings@users.noreply.github.com>
This PR is adapted from the previous one.
The main new addition it makes to this existing
s/read-un-as-sqbranch is treating unknown elements* as bytes not strings. It also contains changes from a main branch rebase.*outside of the implicit syntax+undefined length case