search

ivoflipse / pydicom

Automatically exported from code.google.com/p/pydicom
0 stars 1 forks source link

Skeleton object hierarchy and API definition (skeleton_object_hierarchy.py) #67

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
Quick hack of a condensed object hierarchy to concretize and substantiate 
aspects of the discussion that Darcy started with the "Proposal for 
reorganization of the DataElement types".

As discussed on the mailing lists I already had some ideas regarding the 
object hierarchy, most of which are concisely exemplified in this file.

The most important part of this file supposedly is the doctests in the 
function "test_skeleton_implementation()", which could serve as an API-
specification, whose functionality, possibly performance, etc. could be 
tested in a straightforward manner. Just run the script as "main" to test 
the doctests.

I currently believe that the approach sketched here

1) is or could be made backwards compatible with pydicom
2) concentrates all the "magic" of multiple-style acces on a "custom" top-
level object (DicomPart10File)
3) allows flexible treatment of DataElement values, whether multi-valued 
or not (?)
4) seems to allow all styles of attribute access that Darcy asked for (?) 
without much additional restructuring and or a need to have DataElements 
subclass a variety of builtins (?).
5) does not attempt to stick parts of a dicom file (preamble) onto other 
parts of the file (dataset)

It is well possible that I am missing lots of blatantly obvious things, so 
feel free to just ignore this :-)

DaniN

Original issue reported on code.google.com by DaniN...@gmail.com on 9 Dec 2009 at 3:19

Attachments:

GoogleCodeExporter commented 9 years ago 
Dani,
I really appreciate your efforts on this... and I haven't had a proper chance 
to 
review your code yet. But at the same time, I wanted to continue the discussion 
by 
including the code I had previously mentioned I was working on. A start on that 
has 
been pushed up to another repository clone at 
http://code.google.com/p/pydicom/source/clones. It handles the VR re-working, 
but not 
your comments on Part10 objects.

I've set that repository so anyone can do code reviews, so you could add 
comments 
there or continue discussing here, whichever makes the most sense for the types 
of 
comments, I guess.

Between your efforts and mine (and any others who wish to add thoughts), 
hopefully we 
can come to a consensus and begin making the code changes.

Regards,
Darcy

Original comment by darcymason@gmail.com on 13 Dec 2009 at 7:25

GoogleCodeExporter commented 9 years ago 
I realized I should have added some info in my comment above about the new 
repository 
to point people to the experimental code. The relevant code is in a new 
subdirectory 
called 'vr' under the dicom directory. Also of interest are the test_DS and 
test_IS 
unit tests in the test directory (and I'm working on test_multival which may go 
up 
soon). These files are disconnected from use in pydicom datasets so far. They 
would 
have to be hooked into the Dataset.__setitem__, __setattr__ etc. so that any 
normal 
python types passed in could be coerced to the proper type for the VR of the 
tag 
being set.

The ideas morphed quite a bit while I was working on them -- in the end, I 
wound up 
with the VR's as new types in python, where validation is done right when the 
object 
is created (in __new__). I had thought about leaving validation until the end, 
but 
realized that if someone wanted to write code with non-valid DICOM values, they 
could 
do so with regular python types outside the Dataset, and only add it to the 
dataset 
at the end (by then it had better be a valid DICOM value anyway).

Original comment by darcymason@gmail.com on 14 Dec 2009 at 12:05

GoogleCodeExporter commented 9 years ago 
I decided to use DaniN's skeleton API concept before finishing my changes in 
the experimental repository -- attached is 
a single file which completes my ideas for VR types, showing everything in one 
place (only the new VR type concepts, 
not the Part10File ideas). I didn't use exactly the same function names, etc. 
as DaniN's, because of trying to match the 
existing pydicom names (or what they should be with PEP-8 style) as closely as 
possible.

The __main__ section at the bottom has tests to verify the correct behaviour. 
Single values and multi-value elements are 
both available, with DICOM validity checking (which can be turned off if 
desired).

With this concept, there would be no DataElement objects. The Dataset simply 
stores values derived from VR_base. Since 
the current pydicom DataElement stores VR, VM, tag as well as value, then for 
backwards compatibility, these new VR 
types would support VR, VM and value attributes. VM is not done for the 
MultiValue class in this example, but can be 
trivially added (with len(self)). I think 'value.tag' should not be used 
because a "value" should be pure -- the same one 
could be bound to multiple items having different tags.

I don't think there is a lot of "magic" in this code, and it actually 
simplifies some things by removing the DataElement 
class completely. The DICOM dataset is represented as a python dict of tag: 
value pairs, where the values are essentially 
python types, but are further restricted according to DICOM rules. 

Unless anyone can point out flaws in these concepts, my plan would be to make 
this change (including classes for all VR 
types) for pydicom 0.9.5 (after version 0.9.4 -- the improvements to file 
reading, done in the main repository).

-Darcy

Original comment by darcymason@gmail.com on 26 Dec 2009 at 4:53

Attachments:

GoogleCodeExporter commented 9 years ago 
Changing priority on this, not expecting to revisit it in the short term. The 
current data conversion is working well, except that Decimal could be improved 
(issue 43).

Original comment by darcymason@gmail.com on 16 Dec 2011 at 1:56