Repository: pydicom/pydicom

Issue:
Update filewriter group length fields
_From [darcymason@gmail.com](https://code.google.com/u/darcymason@gmail.com/) on December 10, 2008 22:48:30_

It looks like filewriter module does not recalc group length fields except
for file meta info section. Should make this the case for all groups.

_Original issue: http://code.google.com/p/pydicom/issues/detail?id=30_


Discussion:
darcymason: I had a quick look at this and it looked fine, but then I tried commenting out the 'continue' lines to make the test fail, and got some strange warnings from values.py about unexpected length.

I've been travelling and haven't had a chance to try again or investigate further.  Will look into it when I can, or perhaps someone else can give it a try -- I may have set something up incorrectly.
mrbean-bremen: Hm, if I do the same (e.g. replace `continue` with `pass`) I get:
```
    def test_write_removes_grouplength(self):
        ds = dcmread(color_pl_name)
        assert 0x00080000 in ds
        ds.save_as(self.file_out, write_like_original=True)
        self.file_out.seek(0)
        ds = dcmread(self.file_out)
        # group length has been removed
>       assert 0x00080000 not in ds
E       AssertionError: assert 524288 not in (0008, 0000) Group Length                        UL: 480\n(0008, 0008) Image Type                          CS: ['ORIGIN...Group Length                        UL: 92172\n(7fe0, 0010) Pixel Data                          OW: Array of 92160 bytes
```
which looks ok to me.
darcymason: Okay, I finally cracked this - it had nothing to do with this latest code, but I couldn't leave the mystery.  Well, it is still a little bit of a mystery.

It went away if I removed the other classes from test_filewriter.py.  The second clue was that the error and warnings appeared twice as often as expected.  Finally I noticed there were coming both from class WriteFileTests and from ScratchWriteDateTimeTests.  Then finally noticed that the latter is *derived* from the former.  Changed that to subclass from unittest.TestCase, and my issue went away, and the tests run normally with the `continue` line put back also.  So I haven't tried to figure out exactly why it causes trouble in both classes when the one is derived from the other, but it certainly is the source.  I suspect it is something to do with the temp file not being reset, and maybe the order of events depends on platform (I was testing on Windows, python 3.6.4).

So... @mrbean-bremen, if you don't mind updating that one line, then I'd be happy to merge this.
scaramallion: It looks like the change was made so that the `WriteFileTests` tests are run again but with `config.datetime_conversion = True`
darcymason: Yes, I think you are right, I remember that now that you bring it up.  But somehow that is not working out when the new test fails.  Well, it shouldn't fail, of course, so maybe it is a moot point, but it does seem the code is a bit fragile somehow.  Personally I like the 'test first' philosophy where you write the new unit test (so that it fails) before fixing the main code.
darcymason: ... I'll merge this and add a separate issue for the test code problem.

Patch:
diff --git a/pydicom/filewriter.py b/pydicom/filewriter.py
index 01d9a4911..e283da86f 100644
--- a/pydicom/filewriter.py
+++ b/pydicom/filewriter.py
@@ -458,6 +458,9 @@ def write_dataset(fp, dataset, parent_encoding=default_encoding):
     tags = sorted(dataset.keys())
 
     for tag in tags:
+        # do not write retired Group Length (see PS3.5, 7.2)
+        if tag.element == 0 and tag.group > 6:
+            continue
         with tag_in_exception(tag):
             # write_data_element(fp, dataset.get_item(tag), dataset_encoding)
             # XXX for writing raw tags without converting to DataElement


Test patch:
diff --git a/pydicom/tests/test_filewriter.py b/pydicom/tests/test_filewriter.py
index f2bac9a81..362704140 100644
--- a/pydicom/tests/test_filewriter.py
+++ b/pydicom/tests/test_filewriter.py
@@ -39,6 +39,7 @@ ct_name = get_testdata_files("CT_small.dcm")[0]
 mr_name = get_testdata_files("MR_small.dcm")[0]
 jpeg_name = get_testdata_files("JPEG2000.dcm")[0]
 no_ts = get_testdata_files("meta_missing_tsyntax.dcm")[0]
+color_pl_name = get_testdata_files("color-pl.dcm")[0]
 datetime_name = mr_name
 
 unicode_name = get_charset_files("chrH31.dcm")[0]
@@ -194,6 +195,15 @@ class WriteFileTests(unittest.TestCase):
         ds = dcmread(fp, force=True)
         assert ds[0xFFFFFFFF].value == b'123456'
 
+    def test_write_removes_grouplength(self):
+        ds = dcmread(color_pl_name)
+        assert 0x00080000 in ds
+        ds.save_as(self.file_out, write_like_original=True)
+        self.file_out.seek(0)
+        ds = dcmread(self.file_out)
+        # group length has been removed
+        assert 0x00080000 not in ds
+
 
 class ScratchWriteDateTimeTests(WriteFileTests):
     """Write and reread simple or multi-value DA/DT/TM data elements"""