Repository navigation
Generate conformant 2.25 UUID-derived UIDs - #191
pradeeppeddineni wants to merge 2 commits into
Conversation
pradeeppeddineni
changed the title
blairconrad
left a comment
blairconrad
left a comment
There was a problem hiding this comment.
Thanks, @pradeeppeddineni. Again, sorry for the delay in responding.
One additional point. This is a breaking change, and I'm not entirely sure when I'll want to release it, so don't feel there's a rush to respond to my comment.
| actual = eval("dataset." + element_path) | ||
|
|
||
| assert actual.startswith("2.25.") | ||
| assert len(actual) <= MAX_UI_LENGTH |
There was a problem hiding this comment.
I figure why make up a constant we have to maintain.
pydicom.valuerep.MAX_VALUE_LEN["UI"] is available, but an alternative, which I like, is to check that the remainder represents an int with no leading 0 and that int is a valid uuid5. It's a tighter check than DICOM requires, but would keep us from surprises. Also, no need for a length check then.
There was a problem hiding this comment.
Agreed, that's a nicer check. I dropped the constant and the length assert. The test now checks that everything after "2.25." is digits with no leading 0 and decodes to a version 5 UUID, and the multivalued test uses the same helper.
blairconrad
left a comment
blairconrad
left a comment
There was a problem hiding this comment.
Ah, I like the helper. And checking the variant. I wouldn't've thought of it. Thanks!
I'll noodle a bit on whether I want to break the format in the next release, but really appreciate your work.
Fixes #189.
I went with the approach you suggested in the issue. There's now a fixed DICOGNITO_NAMESPACE UUID in uianonymizer.py, the anonymizer builds a per seed namespace from it once with uuid.uuid5, and each new UI is uuid.uuid5(seed_namespace, original_ui) written out as 2.25., so it's the DICOM PS3.5 B.2 form, it stays deterministic for the same seed and source value, and the longest possible value is 44 characters, well under the 64 limit.
Tests are in test_anonymize_single_instance.py. They check that the identifying UIs start with 2.25., fit in 64 characters and pass pydicom's UID validation, that a multi-valued ReferencedSOPInstanceUID gets distinct valid values, and that the same seed gives the same UIs while a different seed doesn't. 6 of the new tests fail on main and all pass with the change. uv run test.py is clean (ruff format, ruff check, mypy, 336 tests passed).
I also added a line under Fixed in release_notes.md. One thing worth calling out is that anyone who relied on a seed giving the exact same UIs as older versions will see different values after this, since the format itself changed.