moloney / dcmstack

DICOM to Nifti conversion with meta data preservation

Other

72 stars 51 forks source link

Python3 compatibility -- almost there (replace for #47) #61

Closed yarikoptic closed 5 years ago

yarikoptic commented 5 years ago

Sits on top of https://github.com/moloney/dcmstack/pull/47 but also enables travis testing for python 3.5. Seems that only one issue is still flaky with loading some array fields, didnt look in detail. Otherwise tests seems to pass

yarikoptic commented 5 years ago

as promised -- only few tests fail now under 3.5: https://travis-ci.org/moloney/dcmstack/jobs/402939084#L605

yarikoptic commented 5 years ago

ok -- part of the problem is not within dcmstack but within nibabel :-( https://github.com/nipy/nibabel/pull/647 Any advise on what should be done at the level of dcmstack for that would be welcome since ATM I have no immediate clue

codecov-io commented 5 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@ee9168d). Click here to learn what that means. The diff coverage is 75.86%.

@@            Coverage Diff            @@
##             master      #61   +/-   ##
=========================================
  Coverage          ?   80.87%           
=========================================
  Files             ?        8           
  Lines             ?     2112           
  Branches          ?        0           
=========================================
  Hits              ?     1708           
  Misses            ?      404           
  Partials          ?        0

Impacted Files	Coverage Δ
src/dcmstack/utils.py	`100% <100%> (ø)`
src/dcmstack/nitool_cli.py	`40.11% <12.5%> (ø)`
src/dcmstack/dcmstack_cli.py	`60.82% <25%> (ø)`
src/dcmstack/dcmstack.py	`88.49% <82.14%> (ø)`
src/dcmstack/dcmmeta.py	`86.61% <92.5%> (ø)`
src/dcmstack/extract.py	`85.11% <94.11%> (ø)`

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 ee9168d...f25b377. Read the comment docs.

yarikoptic commented 5 years ago

whoohoo -- and here is with coverage+codecov reporting! might need to enable integration at codecov.io to be in full effect, but overall all green so far with the workaround for nibabel. please review/accept @bmoloney

moloney commented 5 years ago

Can you take a look at my branch "py3-compat" and merge here if everything looks good? It improves test coverage and fixes some more issues.

yarikoptic commented 5 years ago

Thanks, will do! Note that you could pay as well into this branch of mine (in my clone) since I allowed edits from maintainers

yarikoptic commented 5 years ago

pushed, along with just a tiny fix in bbbf3fbeffba222bdf4560316ba7383eb69d3890 didn't have yet a chance for a thorough pass, but let's see first how tests behave I guess

KirstieJane commented 5 years ago

Hey @moloney & @yarikoptic! This is a super useful PR so I'm just swinging by to say it would be really awesome to have merged! @islast in my lab has been working from it today with heudiconv so if you need any help with testing etc we could maybe give some feedback? I think its working well so far but it would be really cool to be able to pip install rather than pull from github.

I totally understand you're both really busy so please disregard this ping until you have time to look at it! Thank you!!

yarikoptic commented 5 years ago

Woohoo!!!! Thanks! Now we will just need a release ;-)