suever / pydicom-experimental

pydicom test
0 stars 1 forks source link

UID not equal comparison by description wrong #121

Closed suever closed 9 years ago

suever commented 9 years ago

From darcymason@gmail.com on January 27, 2013 20:50:18

What steps will reproduce the problem? >>> ds=dicom.read_file("CT_small.dcm")

ds.file_meta.MediaStorageSOPClassUID == "CT Image Storage" True # as it should be

Try same comparison with not equal

ds.file_meta.MediaStorageSOPClassUID != "CT Image Storage" True # should be False

ds.file_meta.MediaStorageSOPClassUID != '1.2.840.10008.5.1.4.1.1.2' False # works correctly when compared to UID

Original issue: http://code.google.com/p/pydicom/issues/detail?id=121

suever commented 9 years ago

From Suever@gmail.com on January 28, 2013 08:30:23

Good catch!

I'm assuming the fix should be something as simple as:

def ne(self, obj): return not self == obj

More generally, in pydicom there are a number of classes derived from str. It would probably be best to go through and find those and make sure that they implement the != operator if they overload the == operator.

suever commented 9 years ago

From darcymason@gmail.com on January 28, 2013 10:02:44

You would think the ne definition should be enough, but I had a vague recollection that I read somewhere there is more to it. Plus I had thought in the absence of ne that it would automatically call eq, but according to http://docs.python.org/2/reference/datamodel.html#basic-customization , it does not, and ne should be defined any time eq is.

As to all other class, we have to be careful about hash definition -- it should refer to the parent hash if an immutable (all ours are, I think); if mutable than it should be set to None. http://docs.python.org/3/reference/datamodel.html#object.__hash__ and http://docs.python.org/2/reference/datamodel.html#object.__hash__ So looks like ne is appropriate, but we should have unit tests too, and do this for all classes as Jonathan said.

suever commented 9 years ago

From darcymason@gmail.com on July 19, 2013 15:56:40

Checked for spots that define a eq or ne across all pydicom code $> grep -rHin --include ".py" "eq|ne" UID.py:99: def eq(self, other): UID.py:101: if str.eq(self, other) is True: # 'is True' needed ( issue 96 ) UID.py:103: if str.eq(self.name, other) is True: # 'is True' needed ( issue 96 ) UID.py:122: # For python 3, any override of cmp or eq immutable requires tag.py:58: def eq(self, other): tag.py:67: def ne(self, other): tag.py:76: # For python 3, any override of cmp or eq immutable requires valuerep.py:240: def eq(self, other):

So UID and valuerep.py (class PersonName3) appear to be the only places of concern for this issue.

suever commented 9 years ago

From darcymason@gmail.com on July 19, 2013 16:31:02

This issue was closed by revision 1582bf62a903 .

Status: Fixed