Skip to content

MRG: refactoring for pydicom 1.0 #599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Feb 19, 2018
Merged

MRG: refactoring for pydicom 1.0 #599

merged 3 commits into from
Feb 19, 2018

Conversation

matthew-brett
Copy link
Member

@matthew-brett matthew-brett commented Feb 18, 2018 •
edited
Loading

Make tests pass against pydicom 1.0.

Refactor CI to test pydicom master, as suggested by pydicom folks.

Update minimum pydicom dependency to 0.9.9 (previous versions no longer install).

@codecov-io
Copy link

codecov-io commented Feb 18, 2018 •
edited
Loading

Codecov Report

Merging #599 into master will increase coverage by 1.05%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage    93.4%   94.46%   +1.05%     
==========================================
  Files         189      177      -12     
  Lines       25820    24943     -877     
  Branches     2752     2661      -91     
==========================================
- Hits        24118    23562     -556     
+ Misses       1205      908     -297     
+ Partials      497      473      -24
Impacted Files Coverage Δ
nibabel/nicom/tests/test_dicomwrappers.py 98.34% <100%> (ø) ⬆️
nibabel/info.py 100% <100%> (ø) ⬆️
nibabel/nicom/dicomwrappers.py 90.73% <100%> (ø) ⬆️
nibabel/pydicom_compat.py 60% <83.33%> (+3.75%) ⬆️
nibabel/benchmarks/bench_fileslice.py
nibabel/externals/netcdf.py
nibabel/externals/__init__.py
nibabel/externals/tests/test_six.py
nibabel/benchmarks/bench_arrayproxy_slicing.py
nibabel/benchmarks/bench_finite_range.py
... and 6 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 6eacaf5...618d02a. Read the comment docs.

@coveralls
Copy link

coveralls commented Feb 18, 2018 •
edited
Loading

Coverage Status

Coverage increased (+1.03%) to 96.36% when pulling 618d02a on matthew-brett:pydicom-compatibility into 6eacaf5 on nipy:master.

@matthew-brett matthew-brett force-pushed the pydicom-compatibility branch 3 times, most recently from 58ae278 to 9f9cc3e Compare February 18, 2018 13:22
@matthew-brett matthew-brett changed the title WIP: refactoring for pydicom 1.0 MRG: refactoring for pydicom 1.0 Feb 18, 2018
effigies
effigies reviewed Feb 18, 2018
View reviewed changes
.travis.yml
@@ -34,29 +33,29 @@ matrix:
# Absolute minimum dependencies
- python: 2.7
env:
- DEPENDS="numpy==1.7.1" PYDICOM=0
- DEPENDS="numpy==1.7.1"
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the idea here to make sure that we didn't break when pydicom was absent? Do we want to move to a pydicom dependency?

Copy link
Member

Choose a reason for hiding this comment

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

Wow, brain failure. I see this replaces the pydicom dependency above. Carry on.

Copy link
Member Author

Choose a reason for hiding this comment

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

With this matrix entry, Pydicom isn't in the DEPENDS variable, and doesn't get installed. On the way, the tests did in fact fail when Pydicom wasn't present, I had to fix the code to make it work again. Have a look at the Travis-CI log to confirm this entry does not install Pydicom...

@effigies
Copy link
Member

This looks reasonable. The AppVeyor build seems to have failed for its own reasons (I can't restart it; not sure if that's a thing in AppVeyor).

@matthew-brett
RF: refactoring to account for pydicom 1 changes.
d83d81c 
See also:

* pydicom/pydicom#553
* pydicom/pydicom#554
* pydicom/pydicom#555
@matthew-brett
MAINT: Refactor pydicom CI
7ff109c 
Test against master branch.  Remove awkward special casing for Python 3.
@matthew-brett
MAINT: Update minimum pydicom version
618d02a 
Versions < 0.9.9 now break on install when they try to use http to
install the 'distribute' package.
@matthew-brett
Copy link
Member Author

I reverted a couple of the modifications, because pydicom has fixed them already...

@matthew-brett
Copy link
Member Author

Thanks Chris for the review.

@matthew-brett matthew-brett merged commit cc5bf29 into nipy:master Feb 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@effigies effigies effigies approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matthew-brett @codecov-io @coveralls @effigies