Update filereader.py

[parneshr]

Added chuncked reads when reading an element with fixed length to overcome issues with large data elements. In particular elements over 4Gb result in errant reads and when reading the same elements in smaller chuncks of saw 1Gb, no errors are detected. This should have no performance issues for standard size data elements.

[landscape-bot]

Repository health decreased by 0.06% when pulling 39a9908 on parneshr:master into 43f2784 on darcymason:master.

• 6 new problems were found (including 0 errors and 0 code smells).
• No problems were fixed.

[cancan101]

Is there something better than a string that can be used here?

[darcymason]

I like this chunked-read idea in principle, but I echo cancan101's question about using a string. String concatenation would probably be very slow. Another problem is that the binary file read in python 3 returns a bytes object, so I think using a string will generate a type error in python 3.

There are a couple of other minor things: the landscape-bot new issues (whitespace problems -- just style issues), and the setting of buf_size should happen at the top of the generator, not inside the loop just to optimize speed and be a little cleaner.

[darcymason]

Is the need for this removed if memmap (#139) is implemented?

[mrbean-bremen]

I think that memmap could help to prevent this problem - you could just do the partial reads and handle the concatination outside of pydicom, and would avoid any unnneeded performance degradation. The only downside is that it still has to be implemented...
Also, I'm not entirely sure about the original problem - AFAIK, the maximum size of a DICOM element is 4GB, but here it was stated that it was larger than that.

[darcymason]

AFAIK, the maximum size of a DICOM element is 4GB, but here it was stated that it was larger than that.

Agreed. There are only four bytes at most stored in a dicom file for Length, so max possible value length should be 4 GB.

[mrbean-bremen]

I looked at this again, and I would add this only as an option (probably something like chunk_size) which defaults to None, e.g. no chunked read - if it is needed at all, which has to be decided.
The reason is that it seems not to be needed on most (64 bit) systems, and introduces additional memory and CPU load for large files.
For reading byte strings shall be used, as has been already noted, and instead of concatination the byte strings could be collected in a list and joined at the end - this seems to be the fastet way according to stack overflow (though it probably will double the memory usage during joining - haven't checked this).

[darcymason]

Trying to clean up old issues ... this relates to #222. On reading this again, I remembered that length can be "undefined length" so maybe that allows more than 4 GB (although the original post talks about fixed length elements).

To close this, I think we need a solution for the string questions (does BytesIO work more efficiently?), or a memmap solution.

[mrbean-bremen]

@darcymason - I think this one can be closed, because the problem can be handled by reading frame-per-frame, once the respective PR has been merged. A possible memmap feature would also help, as discussed.

[darcymason]

@mrbean-bremen, agreed. Closing.