Issue 135 in pydicom: Is this a bug in write_file?

[pyd...@googlecode.com]

Status: New
Owner: ----
Labels: Type-Defect Priority-Medium Difficulty-Medium

New issue 135 by bton...@gmail.com: Is this a bug in write_file?
http://code.google.com/p/pydicom/issues/detail?id=135

What steps will reproduce the problem?
(code stub attached)
1. Modify a valid dvh value: ds1.DVHs[0].DVHData[0] = NewFirstPoint
2. Save using ds1.write_file, and read back again into new ds2
3. Read-in data is not updated correctly; ds1.DVHs[0].DVHData[0] <>
ds2.DVHS[0].DVHData[0]. The data stored reflects the ORIGINAL data in ds1
(which means that write_file is getting the data from a shadowed copy of
the data? Subtle.)

What is the expected output? What do you see instead?

Sample code attached. Used a valid RT Dose file with DVH's as a starting
point. Later, tried creating DVH's from scratch, and found a similar
behavior. Once DVHData is appended to DVHs[index], elements of DVHData that
are changed programmatically are not saved with write_file.
Is this a bug, or have I missed a step?

What version of the product are you using?
Tried this with both 0.9.6 and 0.9.7

***NOTE***: any text or attached files posted with the issue can be viewed
by anyone. You are solely responsible to ensure that they contain no
confidential information of any kind.

Please provide any additional information below.
CODE STUB ATTACHED




Attachments:
dvh_rw_error.py 1.2 KB

--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[pyd...@googlecode.com]

Comment #1 on issue 135 by Sue...@gmail.com: Is this a bug in write_file?
http://code.google.com/p/pydicom/issues/detail?id=135

I cannot get your problem to reproduce when manually modifying the
rtplan.dcm in the testfiles folder of pydicom (code attached for
modification).

When running your file and substituting the original rtplan.dcm and
modified version I get:

0.9.7
Original first data point: 1.0
New first data point: 1234.567
Read in saved first point: 1234.567
Point was not updated?: False
Changed point saved correctly?: False

Also, note that at least in 0.9.7, your comparison in the last line of the
script will not be true because NewFirstPoint is a float while
ModifiedFirstPoint is a string since DVHData is of type DS. We changed this
behavior in 0.9.8.

Do you have the same issues when using the most recent version of pydicom?

Also feel free to post a de-identified file which exhibits the problem that
you're seeing so that we can reproduce the error.

Attachments:
add_DVHData.py 217 bytes

[pyd...@googlecode.com]

Comment #2 on issue 135 by bton...@gmail.com: Is this a bug in write_file?
http://code.google.com/p/pydicom/issues/detail?id=135

OK, it could be the DS data type problem, and the float vs. string problem.
I'm on travel for a few days but will test those ideas when I get back. I
think it could well be that my assignments over-ride a data type, like from
string to float, and then this generates a silent error in write_file.
Still not clear on how write_file could possibly "remember" the original
assignment. Does the pydicom object have a "shadow" copy of DVHData?
When I get back I'll post an anonymous version of the troubling data file.
It was a problem with RT_DOSE files from more than one planning system.

[pyd...@googlecode.com]

Comment #3 on issue 135 by bton...@gmail.com: Is this a bug in write_file?
http://code.google.com/p/pydicom/issues/detail?id=135

The problem was not with ds.write_file, but with assignment to an element
of DVHData.

Specifically:
#Make a dataset for the dvh
dvh = dcm.dataset.Dataset()
#Assign the dvh "all at once", which will work properly
dvh.DVHData = ['1.234','5.678']
print type(dvh.DVHData[0]) #prints <class 'dicom.valuerep.DSfloat'>
dvh.DVHData[0] = '8.987'
print type(dvh.DVHData[0]) #prints <type 'str'>

I could not find a "proper" way of assigning to an individual element of
the dataset.
These all failed, storing strings/floats instead of DSfloats, or not
allowing assignment:
dvh.data_element('DVHData').value[0]='3.14' #assigns, but as string
dvh.data_element('DVHData').value[0]=3.14 #assigns, but as float
dvh[0x3004,0x0058][0]='3.14' #does not support assignment

Is there a way to do this properly? The work around was to make a temp
object myDVHData, work with it, and then assign it to dvh.DVHData in one
step. Seems like you should be able to manipulate elements
dvh.DVHData[index] directly, though.

[pyd...@googlecode.com]

Comment #4 on issue 135 by Sue...@gmail.com: Is this a bug in write_file?
http://code.google.com/p/pydicom/issues/detail?id=135

It does seem that there is a type mis-match when you manually assign a
single value of the DVHData DS, and I believe that this behavior was
corrected in the most recent release 0.9.8. On the other hand, when you
save the file, even if it is identifying itself as a str it should save and
then load properly as a DS VR. Did you try this?

In the two of the three cases that you showed for assigning (three of four
if you count the first example), although the type of the value right after
assignment is what you entered, the file saves appropriately. Again, this
is fixed in 0.9.8 but it should still be functional in 0.9.7

Finally, the last way that you tried to do the assignment didn't work
because dvh[0x3004,0x0058] returns a data element which requires you to use
the value property to do the actual assignment so the following would work:

dvh[0x3004,0x0058].value[0]='3.14'

Try saving the file and then reloading to ensure that these work for you.

[pyd...@googlecode.com]

Comment #5 on issue 135 by bton...@gmail.com: Is this a bug in write_file?
http://code.google.com/p/pydicom/issues/detail?id=135

I've tested the simple cases of assignment to an element of DVHData in
0.9.6 and 0.9.8, and yes, the behavior is quite different (in 0.9.6, the
internal representation returned by dataset.data_element is different from
that accessed as a list element).

Also, in 0.9.8 the incorrect type assignment can be "white-washed" by a
write/read cycle, as you suggest. This doesn't seem very efficient, though.

Sticking to just 0.9.8, my feeling about all this is that this behavior
isn't consistent with the project design goal to "to have a single copy of
the data" (quoting from the developer's guide).

I ran into this issue earlier, when I had trouble getting overrides to work
when DS got changed to decimal-string (I now know why that didn't work--my
overrides tested for data element type).

I would like to avoid making copies of all dicom objects, since that just
makes it easy to lose track of them. The current behavior of assignment
(silently accepting a type change that is inconsistent with the VR), isn't
going to work well in my projects. My workaround is to work with copies of
the data, and only use all-at-once assignments of element lists--no
assignments to individual members of the list.

My feeling is that this would all be safer, if pydicom exposed (and
required?) a setter/getter for accessing data elements. Has that been
considered (i.e. explicit get()/set() or get_value/set_value)? I see you're
using "value" as a setter/getter in dataelem. Is it possible to modify
self.value in DataElement to typecast from strings (and floats) to
valuerep.DSfloat, when self.VR == DS?

And, it goes without saying, thanks for a great library.

[pyd...@googlecode.com]

Comment #6 on issue 135 by Sue...@gmail.com: Is this a bug in write_file?
https://code.google.com/p/pydicom/issues/detail?id=135

To follow up on this issue. All but one of the use cases has been corrected
in versions >= 0.9.8. The remaining case where the expected behavior isn't
observer is when the DataElement.value is a list (VM > 1) and the user
attempts to modify this value directly (i.e.
DataElement.value.append(new_value)).

The current implementation of the DataElement.value setter/getter is that
on setting, the value (or values) are converted to the proper type (i.e.
str to DSfloat). When the value of an item is retrieved, no conversion
occurs but rather the _value is returned. This is for performance reasons
as we only want to convert data once.

The issue occurs when the list() used for DataElement.value is modified
directly (via append(), insert() etc), the DataElement.value setter is
never triggered since the list reference never changes, just the data
inside of it. Ideally we would like to trigger the DataElement.value setter
when the CONTENTS of the DataElement.value list are modified.

Unfortunately, it seems like this isn't easy to do apart from creating a
wrapper (or proxy) for the built-in list datatype that overloads the
append(), insert(), __setitem__() methods to convert the input data prior
to inserting it into the list. I've seen some examples using
collections.MutableSequence. I'm not sure this is a good path to go down or
not.