# Scene load broken due to new volume unit attribute

**URL:** <https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771>\
**Category:** Development\
**Tags:** scene\
**Created:** [July 26, 2017, 1:40pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771 "2017-07-26T13:40:10Z")\
**Posts on this page:** 9\
**Page:** 1

<div class="post-metadata">

**Author:** ![cpinter](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/cpinter/32/7995_2.png) [@cpinter](https://discourse.slicer.org/u/cpinter)\
**Post date:** [July 26, 2017, 1:40pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/1 "2017-07-26T13:40:10Z")

</div>

py\_SubjectHierarchyGenericSelfTest started failing due to a failure of loading a scene. Digging into it, this error message (Error parsing XML in stream at line 38, column 151, byte index 10406: not well-formed (invalid token)) points to this line:

```
 <Volume
  id="vtkMRMLScalarVolumeNode1" name="303: Unnamed Series" hideFromEditors="false" selectable="true" selected="false" attributes="DICOM.QuantityCode:{"CodeMeaning": "Attenuation Coefficient", "CodingSchemeDesignator": "DCM", "CodeValue": "110852"};DICOM.UnitsCode:{"CodeMeaning": "Hounsfield unit", "CodingSchemeDesignator": "UCUM", "CodeValue": "[hnsf'U]"};DICOM.instanceUIDs:1.2.826.0.1.3680043.8.274.1.1.2895132669.2819.7830930470.104 1.2.826.0.1.3680043.8.274.1.1.7405746770.5479.5810198465.165 1.2.826.0.1.3680043.8.274.1.1.3116843726.8233.3216805822.278 1.2.826.0.1.3680043.8.274.1.1.3263562271.1129.3152372828.828 1.2.826.0.1.3680043.8.274.1.1.4153098248.6996.3632241357.584 1.2.826.0.1.3680043.8.274.1.1.4770490127.9140.3834798946.219 1.2.826.0.1.3680043.8.274.1.1.6553947155.9980.4127073411.708 1.2.826.0.1.3680043.8.274.1.1.4554129680.8666.4619454551.121 1.2.826.0.1.3680043.8.274.1.1.5613951088.5396.9288000555.812 1.2.826.0.1.3680043.8.274.1.1.5710109910.8056.9834733307.864" displayNodeRef="vtkMRMLScalarVolumeDisplayNode1" references="display:vtkMRMLScalarVolumeDisplayNode1;" userTags="" ijkToRASDirections="-1 0 0 0 -1 0 0 0 1 " spacing="49 49 23" origin="248.844 248.289 -123.75" ></Volume>

```

Specifically the quotation mark before “CodeMeaning”. As this attribute is added when loading a volume from DICOM, I suspect that now all saved scenes are invalid that contain a DICOM volume. It would be good either to revert this commit  
[https://github.com/Slicer/Slicer/commit/9df49830897e2d0d6e458e7390eec7807a817b41](https://github.com/Slicer/Slicer/commit/9df49830897e2d0d6e458e7390eec7807a817b41)  
while fixing this, or fixing this quickly.

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [July 26, 2017, 1:51pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/2 "2017-07-26T13:51:13Z")

</div>

I’ll take care of this by this information into a member variable. We could encode the attribute to be able to store special characters, such as ", but we’ll use the unit for many things, so it’s better not to store it in just a generic metadata.

Scene corruption is a serious error that our development process should have captured. The pull request was open for a few days but just by looking at the code nobody noticed the error. SH test did capture the error (see [failed test this morning](http://slicer.cdash.org/testDetails.php?test=8141795&build=1069200), it even showed the relevant error message on the dashboard “Error parsing XML in stream at line 38, column 151, byte index 10404: not well-formed (invalid token)”) and the dashboard is normally green (we don’t have any failing test), so it should not be too difficult to spot new errors.  
@jcfr do you know why the tests were not executed automatically by the pull request ([https://github.com/Slicer/Slicer/pull/750](https://github.com/Slicer/Slicer/pull/750))?  
@Fedorov do you remember if you’ve run the automatic tests manually and if you saw the new test failure?

---

<div class="post-metadata">

**Author:** ![fedorov](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/fedorov/32/14_2.png) [@fedorov](https://discourse.slicer.org/u/fedorov)\
**Post date:** [July 26, 2017, 2:03pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/3 "2017-07-26T14:03:01Z")

</div>

Sorry for the trouble guys. I admit I did not run the tests manually, so it is my fault!

@lassoan will you implement the solution you mentioned, or should I revert the commit?

---

<div class="post-metadata">

**Author:** ![jcfr](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jcfr/32/17825_2.png) [@jcfr](https://discourse.slicer.org/u/jcfr)\
**Post date:** [July 26, 2017, 2:04pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/4 "2017-07-26T14:04:25Z")

</div>

> [@lassoan](#):
>
> this information into a member variable

Definitively, storing a complex document (e.g json) as a MRML attribute is a stretch.

> [@lassoan](#):
>
> do you know why the tests were not executed automatically by the pull request

Tests are not yet run automatically, we need to finalize integration of the work of Mayeul. For now, it is only the build.

As soon as this is enabled, we will capture such regression beforehand.

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [July 26, 2017, 2:05pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/5 "2017-07-26T14:05:53Z")

</div>

I revert the commit now and implement member variable storage within a few hours.

---

<div class="post-metadata">

**Author:** ![jcfr](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jcfr/32/17825_2.png) [@jcfr](https://discourse.slicer.org/u/jcfr)\
**Post date:** [July 26, 2017, 2:07pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/6 "2017-07-26T14:07:10Z")

</div>

Thanks @lassoan for tackling this.

On my side, I will look into adding the testing to the PR later this week

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [July 26, 2017, 2:10pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/7 "2017-07-26T14:10:11Z")

</div>

Reverted in rev r26176.

---

<div class="post-metadata">

**Author:** ![cpinter](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/cpinter/32/7995_2.png) [@cpinter](https://discourse.slicer.org/u/cpinter)\
**Post date:** [July 26, 2017, 2:11pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/8 "2017-07-26T14:11:54Z")

</div>

Thanks guys!

Do you think it would be worth sanitizing an attribute when setting (and restoring it when getting)? Users might add attributes with quotation marks and they might wonder what broke. Not sure if simply escaping the quotation mark would solve this, but if it does, then it’s a very simple solution.

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [July 26, 2017, 2:14pm UTC](https://discourse.slicer.org/t/scene-load-broken-due-to-new-volume-unit-attribute/771/9 "2017-07-26T14:14:55Z")

</div>

Yes, currently special characters in any node properties (name, description, attributes, …) can lead to invalid scenes - see [https://issues.slicer.org/view.php?id=3406](https://issues.slicer.org/view.php?id=3406). There is a function in MRML node already for encoding/decoding, we could probably use that in read/write XML of MRML node to fix this.