Skip to content

ENH: deprecate Wrapper.get_affine - use affine property #796

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.

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

Merged
merged 2 commits into from
Sep 11, 2019

Conversation

yarikoptic
Copy link
Member

To stay inline with the regular interfaces

If there are maintenance releases to happen with python2 support -- this should probably cherry picked there or I could rebase -- should it be maint/2.5.x?

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #796 into maint/2.5.x will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           maint/2.5.x    #796      +/-   ##
==============================================
- Coverage           90%     90%   -0.01%     
==============================================
  Files               95      94       -1     
  Lines            12045   12044       -1     
  Branches          2142    2142              
==============================================
- Hits             10841   10840       -1     
  Misses             857     857              
  Partials           347     347
Impacted Files Coverage Δ
nibabel/nicom/dicomwrappers.py 90.28% <100%> (+0.07%) ⬆️
nibabel/_h5py_compat.py
nibabel/minc2.py 90.12% <0%> (+0.12%) ⬆️

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 457c860...0932c6d. Read the comment docs.

yarikoptic added a commit to neurodebian/dcmstack that referenced this pull request Aug 23, 2019
@effigies
Copy link
Member

Easiest is to rebase on the maintenance branch. We can still merge that branch into master pretty easily. Can move to a backport approach once that gets painful.

@yarikoptic
Copy link
Member Author

Easiest is to rebase on the maintenance branch.

maint/2.5.x ?

@yarikoptic
Copy link
Member Author

ok, tests against master passed. rebased, force pushed and now will change the base in the PR to maint/2.5.x

@yarikoptic yarikoptic changed the base branch from master to maint/2.5.x August 23, 2019 18:06
@effigies
Copy link
Member

Looks like CI got confused about the changing target branch. Bumped.

@effigies
Copy link
Member

effigies commented Aug 23, 2019

Thanks, @yarikoptic. Would you mind updating the class docstring to note that affine is now a property and remove get_affine?

On the whole, it looks like the class could use some conforming to the SpatialImage API, but I don't want to expand the scope of this PR. I'll open an issue.

@effigies
Copy link
Member

@yarikoptic If you care to update the docstring, this could go in the next release, which should be a week or so.

@yarikoptic
Copy link
Member Author

Sorry @effigies for not reacting, thanks for pushing the tune up!

@effigies
Copy link
Member

The AppVeyor failures are fixed in #804, so the easiest fix is to merge that and merge or rebase maint/2.5.x.

@yarikoptic
Copy link
Member Author

please proceed any way you see best fit ;)

@effigies
Copy link
Member

@yarikoptic Good to merge?

@yarikoptic
Copy link
Member Author

Sure, why not? ;-)

@yarikoptic yarikoptic merged commit f47f80e into nipy:maint/2.5.x Sep 11, 2019
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.

2 participants