[MRG] Support for pydicom v3

[medihack]

Reference issue

Support for pydicom v3, but drops support for pydicom v2.

Tasks

• Unit tests added that reproduce issue or prove feature is working
• Fix or feature added
• Documentation and examples updated (if relevant)
• Unit tests passing and coverage at 100% after adding fix/feature
• Type annotations updated and passing with mypy
• Apps updated and tested (if relevant)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (bc5f1ae) to head (aefbb72).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #972   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines         8708      8707    -1     
=========================================
- Hits          8708      8707    -1     

Files with missing lines	Coverage Δ
pynetdicom/association.py	100.00% <100.00%> (ø)
pynetdicom/dsutils.py	100.00% <100.00%> (ø)
pynetdicom/events.py	100.00% <100.00%> (ø)

[medihack]

The coverage went down a bit as the original_encoding property with pydicom v3 is automatically set (in contrast to pydicom v2) when a dataset is loaded. That way this if block is not reachable anymore with the loaded dataset in test_dataset_encoding_mismatch (for example ds.is_implicit_VR = True doesn't influence ds_encoding anymore).
The original_encoding is (None, None) in a manually created dataset. So, in contrast to pydicom v2, original_encoding is always set in pydicom v3.

[medihack]

This should hopefully have 100% coverage again. There is one caveat. It behaves differently than before when loading a dataset (because original_encoding seems to be always present under pydicom v3), but I think that is the way it should be. The comment in the code dataset might also be created from scratch makes more sense to me now.

[medihack]

Good to know ... ruff formatting is not fully compatible with black. But for that conda error I am unguilty 😉.

[mrbean-bremen]

ruff formatting is not fully compatible with black

Yeah, it probably makes sense to introduce pre-commit into this repo like in pydicom, so the same tests can be run locally as in the CI.

Conda seemed to have a temporary problem - I restarted the test, and it seems to work now.

[mrbean-bremen]

BTW, I think you can remove this "WIP" - I'll leave the actual review to @scaramallion, but for me it all looks good!

[medihack]

One thing that just came to my mind. Now that I create the dataset manually in test_dataset_encoding_mismatch we should maybe add another test that explicitly shows that the original_encoding is used when loading a dataset and the is_implicit_VR and is_little_endian are ignored then. And should also show some warning when there is a mismatch. So I am not fully happy with the PR and revise it a bit. Sorry for the long chain-of-thought in this PR 😉

[medihack]

After much thought, I would like to propose a change where the transfer syntax is always preferred over the encoding of the dataset in the send_c_store method. But this would be a breaking change. Currently, the now deprecated Dataset.is_implicit_VR and Dataset.is_little_endian can override the transfer syntax (but only if both are set). With the proposed change, the encoding would be selected more similarly to the dcmwrite function of pydicom v3 (resp. the _determine_encoding function).

This means that the priority of the selected encoding in send_c_store would be

1. The encoding of the transfer syntax in the file_meta
2. Dataset.is_implicit_VR and Dataset.is_little_endian (if 1. is not present)
3. Dataset.original_encoding (if 1. and 2. is not present)

I don't have a strong opinion about this. That's why I want to discuss this before implementing it. If this change is unwanted I will of course do it in a different way, but we need a clear way of how to handle the encoding in send_c_store now that Dataset.is_implicit_VR and Dataset.is_little_endian are deprecated and original_dataset is always set.

[scaramallion]

file_meta.set_original_encoding(False, True)

[scaramallion]

Remove the assert

[scaramallion]

Use ds.set_original_encoding(...)

[scaramallion]

After much thought, I would like to propose a change where the transfer syntax is always preferred...

The actual encoding of the dataset is what's important when sending, as we need to determine if it matches the agreed upon transfer syntax, and if not then convert the dataset to match.

For example, if a dataset:

• Had Implicit VR Little Endian as the Transfer Syntax UID
• Was actually encoded as explicit VR
• The agreed upon transfer syntax is Explicit VR Little Endian

Then the dataset can be sent as-is with no conversion. If the agreed upon transfer syntax were instead Implicit VR Little Endian it would need to be read into memory and converted to implicit VR prior to sending.

If the dataset is new, then you can just use the agreed upon transfer syntax.

[medihack]

Thanks a lot for the explanation. I think I understood it (at least I hope so). I have integrated your code comments and adjusted the test_dataset_encoding_mismatch to be more like before.

[scaramallion]

Thanks!