UID generator?

[Félix C. Morency]

Hi,

I've been looking forward to creating DICOM files with pydicom. Other
libraries such as GDCM [1] and DCMTK [2] have utility method to
generate UID to be assigned to 'MediaSOPInstanceUID', for example. The
pydicom source code doesn't seems to contain such a generator. How do
you guys handle UID generation?

Regards,
-Félix

[1]: http://gdcm.sourceforge.net/html/classgdcm_1_1UIDGenerator.html
[2]: http://support.dcmtk.org/docs/dcuid_8h-source.html

[Darcy Mason]

On May 10, 11:15 am, Félix C. Morency <felix.more...@gmail.com> wrote:
> I've been looking forward to creating DICOM files with pydicom. Other
> libraries such as GDCM [1] and DCMTK [2] have utility method to
> generate UID to be assigned to 'MediaSOPInstanceUID', for example. The
> pydicom source code doesn't seems to contain such a generator. How do
> you guys handle UID generation?

Pydicom doesn't have a mechanism for UID generation. There are some
comments at the end of the UID.py file [1] about how to get a UID
root, but to offer sub-roots to users would require some mechanism for
ensuring uniqueness -- either keeping track of sub-roots assigned to
users, or some other mechanism. I haven't looked at those other
libraries to see how they do it, but I would think it has to be a bit
of a headache. If you get your own UID root you can create whatever
scheme you want for ensuring unique IDs. If there's an easy way for
pydicom to do it, let me know and I will see about adding it.

[1] http://code.google.com/p/pydicom/source/browse/source/dicom/UID.py

[Félix C. Morency]

Darcy,

Thanks for your reply. The DCMTK generator [1] (line 1642) uses host
identifier, process id and time to generate a unique identifier
(appended to the root UID). I think it is safe to assume that the
generated UID is unique enough. I will translate this generator to
Python and submit it to you for review.

-Félix

[1]: http://git.dcmtk.org/web?p=dcmtk.git;a=blob;f=dcmdata/libsrc/dcuid.cc;h=528fb2fa0a8054fd10699e0bf28445a5f61cf6c2;hb=HEAD

> --
> You received this message because you are subscribed to the Google Groups "pydicom" group.
> To post to this group, send email to pyd...@googlegroups.com.
> To unsubscribe from this group, send email to pydicom+u...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/pydicom?hl=en.
>



--
--
Félix C. Morency, M.Sc.
Plateforme d’analyse et de visualisation d’images
Centre Hospitalier Universitaire de Sherbrooke
Centre de recherche clinique Étienne-Le Bel
Local Z5-3031 | 819.346.1110 ext 16634

[christian haselgrove]

Félix, Darcy,

[Sorry if this comes through twice; google groups appears to have
eaten my original post.]

It is possible to create your own UID without a unique root: prepend
"2.25" to the integer representation of a UUID. I have used:

'2.25.%d' % uuid.uuid1().int

in the past. I can't link this scheme to the official DICOM standard,
but since it appears on David Clunie's site:

http://www.dclunie.com/medical-image-faq/html/part2.html#UID

I'd say it's de facto standard. :)

Christian

[Félix C. Morency]

Darcy, Christian,

Thank you for pointing that. I pushed a UID generator method in my
clone [1] allowing DICOM UID generation with both DCMTK-like method
and the method found on David Clunie's website.

Please provide feedback/improvement.

Regards,
-Félix

[1]: http://code.google.com/r/felixmorency-uidgen/source/browse

[Félix C. Morency]

Darcy, Christian,

Any thoughts on this?

-F

[Haselgrove, Christian]

Félix,

Sorry for the delayed reply.

> Thank you for pointing that. I pushed a UID generator method in my
> clone [1] allowing DICOM UID generation with both DCMTK-like method
> and the method found on David Clunie's website.
>
> Please provide feedback/improvement.

Looks generally good to me; I like that you can specify a different prefix.

The :max_uid_len truncation shouldn't be necessary for the 2.25 scheme (since UUIDs are 128 bits long), and nor should it for the other scheme, but it does make me slightly nervous that it could mask an unforseen error in the future. If the passed prefix is long enough, it could certainly result in a UID ending in "." (though I can't say definitely that isn't allowed). You might also validate a passed prefix; I could easily see myself forgetting the trailing "." in a custom prefix, for instance.

c

--
Christian Haselgrove
christian....@umassmed.edu
508-856-5363
Fax: 508-856-8211

[Eli Stevens (Gmail)]

I've only followed the thread in passing, but in general, silent
modification of data shouldn't happen by default.

I'd much rather have an API call raise an exception than change
something without notification. If the functionality is really
needed, perhaps have a truncate=False flag that does the modification
when set to True?

Eli

[Félix C. Morency]

@Christian: Thanks for pointing that out!

I agree with Eli on the fact that the data shouldn't be modified
without notice. The flag solution could fit our needs. I will push a
fix soon.

-F

[Darcy Mason]

On May 23, 5:51 pm, Félix C. Morency <felix.more...@gmail.com> wrote:
> @Christian: Thanks for pointing that out!
>
> I agree with Eli on the fact that the data shouldn't be modified
> without notice. The flag solution could fit our needs. I will push a
> fix soon.

I'll definitely pull this into pydicom when the details are worked
out.

On a related note, is it possible to write some basic unit tests for
this? They couldn't check for specific UIDs returned, but perhaps for
handling prefix with/without trailing dot, raising the exception where
appropriate, any possible border cases?

Another thought as I look at the code again ... I think the string
should be returned as a UID class instance rather than a regular
string. At the moment the UID class doesn't offer anything for this
use case, but perhaps in future it might (e.g. could check for valid
values on initialization).

One more little thing -- I've been trying to convert to the
'modern' (standard in python 3) format() function for string
expressions, rather than the % string interpolation. Do you mind
adding that as another minor change?

- Darcy

[Félix C. Morency]

Thank you for your answers. I will push some fixes/tests according to
your comments as soon as possible.

-F

[Félix C. Morency]

Folks,

Sorry for the delay. I've pushed a new set of changes concerning the
UID generator. Please review and make comments.

Regards,
-F

[Darcy Mason]

On Jun 5, 10:22 am, Félix C. Morency <felix.more...@gmail.com> wrote:
> Sorry for the delay. I've pushed a new set of changes concerning the
> UID generator. Please review and make comments.

Hi Félix,

I pulled the changes to my local repository and had a quick look.
Looks fine, but in running the ./shell_all command to run the test
suite in all supported python versions, it generates an error in
python 2.6 (works fine in 2.7):

ERROR: testGenerateUID (dicom.test.test_UID.UIDtests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/darcy/hg/pydicom/source/dicom/test/test_UID.py", line
74, in testGenerateUID
with self.assertRaises(InvalidUID):
TypeError: failUnlessRaises() takes at least 3 arguments (2 given)

Hopefully this is an easy fix ... I'm not familiar with using the
'with' statement for unit tests; perhaps that isn't supported the same
way in python2.6?

Cheers,
Darcy

[Jonathan Suever]

Here's a patch that should fix it. The behavior of with is very inconsistent across releases so I'd recommend avoiding it. You can simply pass a function object to assertRaises to ensure that a certain exception is raised. This can be done either by defining a subfunction or using lambda as I did in the patch.

-Jonathan

Darcy

[Félix C. Morency]

Jonathan, Darcy,

Thank you for pointing that and thank you for the fix.

@Darcy, do you need any manipulation from me or if you can apply the
fix directly on your personal branch?

-F

[Darcy Mason]

On Jun 8, 9:09 am, Félix C. Morency <felix.more...@gmail.com> wrote:
> @Darcy, do you need any manipulation from me or if you can apply the
> fix directly on your personal branch?
>

Jonathon's patch has been applied and all changesets pushed to the
main repository.

Thanks,
Darcy