Skip to content

Conversation

isolovey
Copy link

@isolovey isolovey commented Nov 7, 2018

Increase item limit to 400 to allow parsing larger headers. Resolves #212

@codecov-io
Copy link

codecov-io commented Nov 7, 2018

Codecov Report

Merging #270 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
+ Coverage   67.51%   67.54%   +0.02%     
==========================================
  Files          30       30              
  Lines        2423     2425       +2     
==========================================
+ Hits         1636     1638       +2     
  Misses        787      787
Impacted Files Coverage Δ
heudiconv/dicoms.py 59.83% <100%> (+0.33%) ⬆️

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 f3a7e50...fe1e90b. Read the comment docs.

lgr.info('Filtering out {0} dicoms based on their filename'.format(
nfl_before-nfl_after))
import nibabel.nicom.csareader
nibabel.nicom.csareader.MAX_CSA_ITEMS = 400
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok with me, but may be we should look into either changing it in nibabel itself?
if we are to patch it here, then we should make it conditional like

nibabel.nicom.csareader.MAX_CSA_ITEMS = max(nibabel.nicom.csareader.MAX_CSA_ITEMS, 400)

so we do not bring it down if it is already set higher! but again -- better be changed etc in nibabel since what if they just drop/rename that variable? then our change would either be of no effect

@isolovey
Copy link
Author

isolovey commented Nov 7, 2018

Good point, this really belongs in nibabel. Started a pull request with nibabel instead.

@isolovey isolovey closed this Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants