Skip to content
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

Replace get_affine with affine and get_header with header. #1157

Merged
merged 6 commits into from Dec 16, 2016

Conversation

arokem
Copy link
Contributor

@arokem arokem commented Nov 29, 2016

This (potentially) avoids a lot of annoying deprecation warnings that have started showing up recently.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.004% when pulling af85a86 on arokem:deprecated-nib into e87b327 on nipy:master.

@MarcCote
Copy link
Contributor

So, should we drop support for Nibabel 1.2?

@arokem
Copy link
Contributor Author

arokem commented Nov 29, 2016

Were these properties introduced in 1.3?

@matthew-brett : what is your feeling about raising minimal required version of nibabel?

@matthew-brett
Copy link
Contributor

I think you'll need nibabel >= 2.0. I released 2 early December 2014, and it's a pure Python module without dependencies other than numpy, so raising the version seems reasonable to me.

@codecov-io
Copy link

codecov-io commented Nov 29, 2016

Current coverage is 85.46% (diff: 50.00%)

Merging #1157 into master will not change coverage

@@             master      #1157   diff @@
==========================================
  Files           214        214          
  Lines         24917      24917          
  Methods           0          0          
  Messages          0          0          
  Branches       2526       2526          
==========================================
  Hits          21296      21296          
  Misses         2989       2989          
  Partials        632        632          

Powered by Codecov. Last update e87b327...9b2199c

@arokem
Copy link
Contributor Author

arokem commented Nov 29, 2016

Thanks. I have raised the dependency to 2.0 (in the travis config, as well as the requirements file). I couldn't find a nibabel version specified in any of the documentation, but I might be missing something.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.004% when pulling d55aa3b on arokem:deprecated-nib into e87b327 on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.004% when pulling 9b2199c on arokem:deprecated-nib into e87b327 on nipy:master.

@Garyfallidis
Copy link
Contributor

Garyfallidis commented Dec 1, 2016

A question here. The nibabel.streamlines API was added in 2.0 or in 2.1? If it was added in 2.1 I suggest to have that one as minimum. Small change - big win.

@arokem
Copy link
Contributor Author

arokem commented Dec 13, 2016

Hey @MarcCote : could you please weigh in here? Should we make a slow transition to the new nibabel streamline API, or should we make a clean break here, and start requiring nibabel 2.1? I can see advantages to either, but am particularly worried about moving to a new and relatively API that hasn't been battle tested by many users yet. What do you think?

@jchoude
Copy link
Contributor

jchoude commented Dec 13, 2016

@arokem If I can chime in, the old trackvis functionalities are still available in Nibabel 2.1. This might ease the transition.

@MarcCote
Copy link
Contributor

@jchoude is right, the old trackvis is still available and not yet deprecated (it probably will be in the next release, though). Users must explicitly load their streamlines with the new interface if they want to use it. So, it is safe to require nibabel 2.1, nothing will break as far as streamlines manipulation is concerned.

@arokem
Copy link
Contributor Author

arokem commented Dec 14, 2016

Thanks both. So this is done from my point of view. The codecov failure seems bogus to me. Does someone want to review/merge?

@MarcCote
Copy link
Contributor

So, we stay with Nibabel 2.0 as the minimum requirement? I had the feeling everyone was okay to bump it to 2.1. Other than that and the codecov (I agree it seems bogus), everything looks good to me.

@arokem
Copy link
Contributor Author

arokem commented Dec 16, 2016

Thanks for nudge. I just pushed an upgrade to the minimal requirement on travis to 2.1.0.

Let's see how that goes. I'll make another commit enforcing this elsewhere (requirements, docs)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.004% when pulling ac1fe59 on arokem:deprecated-nib into e87b327 on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.004% when pulling ac1fe59 on arokem:deprecated-nib into e87b327 on nipy:master.

@arokem
Copy link
Contributor Author

arokem commented Dec 16, 2016

As soon as someone reviews/merges this, we can also go ahead and merge #1159 and #1076.

And wouldn't that be nice?

@MarcCote
Copy link
Contributor

Ok, let's merge it. :)

@MarcCote MarcCote merged commit 83ebf37 into dipy:master Dec 16, 2016
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.

None yet

7 participants