Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

write: pad certain VRs with spaces instead of null bytes #81

Merged
merged 1 commit into from
May 30, 2020
Merged

write: pad certain VRs with spaces instead of null bytes #81

merged 1 commit into from
May 30, 2020

Conversation

selimyoussry
Copy link
Contributor

make output:

make
go mod download
/Library/Developer/CommandLineTools/usr/bin/make test
go test ./...
ok  	github.com/suyashkumar/dicom	0.389s
?   	github.com/suyashkumar/dicom/cmd/dicomutil	[no test files]
?   	github.com/suyashkumar/dicom/constants	[no test files]
ok  	github.com/suyashkumar/dicom/dicomio	(cached)
?   	github.com/suyashkumar/dicom/dicomlog	[no test files]
ok  	github.com/suyashkumar/dicom/dicomtag	(cached)
ok  	github.com/suyashkumar/dicom/dicomuid	(cached)
?   	github.com/suyashkumar/dicom/element	[no test files]
?   	github.com/suyashkumar/dicom/frame	[no test files]
ok  	github.com/suyashkumar/dicom/query	0.018s
ok  	github.com/suyashkumar/dicom/write	(cached)
go build -o build/dicomutil ./cmd/dicomutil

On the DICOM file where I experience issue 80, reading the DICOM file with suyashkumar/dicom, and then making an exact copy with this library, I get the following differences when opening in Pydicom:

In [8]: a = pydicom.dcmread(a_fp)                                                                                                       

In [9]: b = pydicom.dcmread(b_fp)                                                                                                       

In [10]: a.RequestAttributesSequence[0]                                                                                                 
Out[10]: 
(0040, 0007) Scheduled Procedure Step Descriptio LO: 'G2702'
(0040, 0009) Scheduled Procedure Step ID         SH: 'G2702'
(0040, 1001) Requested Procedure ID              SH: '918081'

In [11]: b.RequestAttributesSequence[0]                                                                                                 
Out[11]: 
(0040, 0007) Scheduled Procedure Step Descriptio LO: 'G2702\x00'
(0040, 0009) Scheduled Procedure Step ID         SH: 'G2702\x00'
(0040, 1001) Requested Procedure ID              SH: '918081'

After the fix in this PR, and running the same experiment, these fields are now identical in pydicom:

In [6]: a.RequestAttributesSequence[0]                                                                                                  
Out[6]: 
(0040, 0007) Scheduled Procedure Step Descriptio LO: 'G2702'
(0040, 0009) Scheduled Procedure Step ID         SH: 'G2702'
(0040, 1001) Requested Procedure ID              SH: '918081'

In [7]: b.RequestAttributesSequence[0]                                                                                                  
Out[7]: 
(0040, 0007) Scheduled Procedure Step Descriptio LO: 'G2702'
(0040, 0009) Scheduled Procedure Step ID         SH: 'G2702'
(0040, 1001) Requested Procedure ID              SH: '918081'

No more extra NULL byte \x00.

@selimyoussry
Copy link
Contributor Author

Addresses #80

@suyashkumar suyashkumar self-requested a review May 28, 2020 18:22
@selimyoussry
Copy link
Contributor Author

Also @suyashkumar , when you review my PR and it gets accepted and merged, can you please take care of the go.mod version bump? Thanks

suyashkumar
suyashkumar approved these changes May 28, 2020
View reviewed changes
Copy link
Owner

@suyashkumar suyashkumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, just had one minor requested change. After that we should be good to merge.

By the go.mod version bump, you mean tagging the this repo with a new version? Yep, I'll take care of that after merging!

write/writer.go Outdated Show resolved Hide resolved
@suyashkumar suyashkumar self-assigned this May 28, 2020
@suyashkumar suyashkumar added the enhancement New feature or request label May 28, 2020
@selimyoussry selimyoussry force-pushed the issue-80-character-string-vr-use-space-padding branch from 96ff260 to 522d3ec Compare May 28, 2020 18:46
@selimyoussry selimyoussry reopened this May 28, 2020
@selimyoussry
Copy link
Contributor Author

@suyashkumar you'll need to merge this yourself I believe since I don't have write access. Thx!

@suyashkumar suyashkumar changed the title [ISSUE-80] Values with VRs constructed of character strings are padde… [ May 30, 2020
@suyashkumar suyashkumar changed the title [ write: pad certain VRs with spaces instead of null bytes May 30, 2020
@suyashkumar
Copy link
Owner

Sounds good, I’ll merge this now and will comment here when I add the tag (will do so when I get back to my computer).

@suyashkumar suyashkumar merged commit ea802cf into suyashkumar:master May 30, 2020
suyashkumar pushed a commit that referenced this pull request May 30, 2020
@selimyoussry @suyashkumar
write: pad certain VRs with spaces instead of null bytes (#81)
2daf5f9 
Co-authored-by: Selim Youssry <selim@ferrumhealth.com>
@suyashkumar suyashkumar mentioned this pull request May 31, 2020
Closed
@selimyoussry
Copy link
Contributor Author

Sounds good, I’ll merge this now and will comment here when I add the tag (will do so when I get back to my computer).

Ok thanks @suyashkumar , looking forward to it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suyashkumar suyashkumar suyashkumar approved these changes

Assignees

@suyashkumar suyashkumar

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@selimyoussry @suyashkumar