BF: Fix hardcoded maximum number of CSA tags

[hbraunDSP]

It looks like a hardcoded limit of 128 CSA tags was left over in one place after raising it to 1000 everywhere else. This fixes that oversight.

[pep8speaks]

Hello @hbraunDSP, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 nibabel.

Comment last updated at 2019-09-03 14:26:23 UTC

[codecov]

Codecov Report

Merging #798 into master will decrease coverage by 7.88%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #798      +/-   ##
==========================================
- Coverage   90.08%   82.19%   -7.89%     
==========================================
  Files          95       95              
  Lines       11903    11903              
  Branches     2124     2124              
==========================================
- Hits        10723     9784     -939     
- Misses        835     1816     +981     
+ Partials      345      303      -42

Impacted Files	Coverage Δ
nibabel/nicom/csareader.py	68.88% <0%> (-15.56%)	⬇️
nibabel/nicom/utils.py	9.09% <0%> (-90.91%)	⬇️
nibabel/viewers.py	10.52% <0%> (-84.87%)	⬇️
nibabel/processing.py	34.66% <0%> (-65.34%)	⬇️
nibabel/dft.py	19.32% <0%> (-61.66%)	⬇️
nibabel/nicom/dicomwrappers.py	33.07% <0%> (-57.11%)	⬇️
nibabel/minc2.py	35.8% <0%> (-54.33%)	⬇️
nibabel/pydicom_compat.py	50% <0%> (-50%)	⬇️
nibabel/nicom/dicomreaders.py	15.46% <0%> (-43.3%)	⬇️
nibabel/environment.py	75% <0%> (-20%)	⬇️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e684bcc...250ccf8. Read the comment docs.

[effigies]

This looks reasonable to me, but I don't know enough about the format. @matthew-brett Do you know if this 128 limit was supposed to be format imposed, or was just considered a reasonable upper bound at the time?

[matthew-brett]

Yes, just reasonable upper bound. We were just building the parser, and wanted to make sure it didn't run away with us, if the format was not as we expected.

[effigies]

Great. @hbraunDSP is there any chance you have a test file you'd be able to share with us to add to our test suite?

[hbraunDSP]

I don't have a test file but I'll definitely look into writing one.

[effigies]

To be clear, I meant a DICOM file that we can add to our battery that will cause the existing tests to hit this code path, not necessarily writing a new test.

[hbraunDSP]

I do have a file that generated an error and prompted me to look into this. As soon as I can confirm it doesn't contain any PHI or other sensitive data I'll add it.

[effigies]

I'm realizing that we're not actually going to cover the missing lines codecov is complaining about. The pass condition was already being hit, and we're making it less likely to hit the error condition. So at best it would be a check that we're no longer failing on files with 129-1000 tags, which is not a very clear gain.

Sorry for the trouble. I'll merge after tests pass.

[hbraunDSP]

No problem. I might still make a header with exactly MAX_CSA_ITEMS=1000 tags just to be thorough, but thanks for your help!

[effigies]

If you do want to submit a test file, please send it to https://github.com/effigies/nitest-dicom/.