Skip to content

[storescu] prefer file's transfer syntax#520

Merged
Enet4 merged 1 commit intoEnet4:masterfrom
PierreBou91:bug/storescu/ts-selection
Jul 13, 2024
Merged

[storescu] prefer file's transfer syntax#520
Enet4 merged 1 commit intoEnet4:masterfrom
PierreBou91:bug/storescu/ts-selection

Conversation

@PierreBou91
Copy link
Copy Markdown
Contributor

Hi,

As addressed in #473 , I have slightly adapted the check_presentation_context() function to return early if there is an exact match between the file's TS and the TS from the presentation contexts slice that is passed to the function. Otherwise, the previous logic applies. When testing, the selected_ts is consistently the same as the file's TS.

Is it the expected behavior for storescu ?

Additionally, you commented:

(...)
The other criterion currently written is to transcode and accept Explicit VR Little Endian otherwise (only when --never-transcode is not enabled).
(...)
Originally posted by @Enet4 in #473 (comment)

I am uncertain how to address this point and would appreciate some guidance please.

Have a nice day !

@Enet4 Enet4 added A-lib Area: library C-ul Crate: dicom-ul labels Jun 4, 2024
@Enet4 Enet4 self-requested a review June 4, 2024 07:42
@Enet4 Enet4 added A-tool Area: tooling C-storescu Crate: dicom-storescu and removed A-lib Area: library C-ul Crate: dicom-ul labels Jun 4, 2024
@Enet4
Copy link
Copy Markdown
Owner

Enet4 commented Jun 4, 2024

Is it the expected behavior for storescu ?

That seems to do the trick. 👍

Additionally, you commented:

(...)
The other criterion currently written is to transcode and accept Explicit VR Little Endian otherwise (only when --never-transcode is not enabled).
(...)
Originally posted by @Enet4 in #473 (comment)

I am uncertain how to address this point and would appreciate some guidance please.

This should be already in place by the logic that follows.

In the face of uncertainty, we could begin the process of moving key components of dicom-storescu into a new library dicom_storescu and write tests for it.

@PierreBou91
Copy link
Copy Markdown
Contributor Author

That seems to do the trick. 👍

Ok great, thanks for the review !

In the face of uncertainty, we could begin the process of moving key components of dicom-storescu into a new library dicom_storescu and write tests for it.

That sounds like a good idea ! I can start working on it in this PR if you agree ?

I think I will try to mimic the structure of the dump lib. If you think of other resources that might be relevant I'll gladly take a look.

I might have some questions about tests best practices later but I need to take some time to research a bit and try things by myself first. So I'll get back with a first simple commit and probably some questions as soon as I have time !

@Enet4
Copy link
Copy Markdown
Owner

Enet4 commented Jul 6, 2024

Hello @PierreBou91! I don't know about the progress of the storescu lib-bin split, but given the relatively small and useful changes here that can be tested ad-hoc, it would be nice if they were made ready for review and the planned refactor deferred to a new PR.

@PierreBou91
Copy link
Copy Markdown
Contributor Author

Hello @Enet4 ! Sorry I have not started the lib split yet, work has been a bit more intense that I was expecting lately.

You're right, I'll put this PR as ready for review and open a new one at a later date to adress the split when I have a moment !

@PierreBou91 PierreBou91 marked this pull request as ready for review July 6, 2024 11:31
Copy link
Copy Markdown
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

OK, these changes provide an acceptable means of making presentation context selection deterministic: It will prefer the same transfer syntax as the file first, then (only if transcoding is supported) Explicit VR LE second, then Implicit VR LE third.

Much appreciated! 👍

@Enet4 Enet4 merged commit 1692c52 into Enet4:master Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-tool Area: tooling C-storescu Crate: dicom-storescu enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-deterministic behavior in dicom-storescu function check_presentation_contexts

2 participants