Skip to content

FIX: Copy affine_to_rasmm when slicing a Tractogram object #462

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
Jun 27, 2016

Conversation

MarcCote
Copy link
Contributor

This PR simply makes sure to set the affine_to_rasmm when creating a new Tractogram object that is the result of advanced indexing or slicing. It also adds a test to check it is working properly.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage increased (+0.0009%) to 96.112% when pulling a505b72 on MarcCote:fix_tractogram_slicing into f4a3e0a on nipy:master.

@@ -373,6 +374,13 @@ def test_tractogram_getitem(self):
DATA['tractogram'].data_per_streamline[::-1],
DATA['tractogram'].data_per_point[::-1])

# Make sure slicing conserve the affine_to_rasmm property.
Copy link
Member

Choose a reason for hiding this comment

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

Typo conserves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@matthew-brett
Copy link
Member

Other than tiny typo, looks good, thanks.

@codecov-io
Copy link

codecov-io commented Jun 23, 2016

Current coverage is 94.11%

Merging #462 into master will increase coverage by <.01%

@@             master       #462   diff @@
==========================================
  Files           160        160          
  Lines         21164      21169     +5   
  Methods           0          0          
  Messages          0          0          
  Branches       2265       2265          
==========================================
+ Hits          19919      19924     +5   
  Misses          823        823          
  Partials        422        422          

Powered by Codecov. Last updated by f4a3e0a...264d16a

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage increased (+0.0009%) to 96.112% when pulling 264d16a on MarcCote:fix_tractogram_slicing into f4a3e0a on nipy:master.

@@ -18,6 +18,7 @@

def setup():
global DATA
DATA['rng'] = np.random.RandomState(1234)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you're seeding with a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless nosetest sets the seed of np.random I prefer having reproducible tests. In practice, it should work with any seed, so there is no reason really. We also use a seed in test_array_sequence.py.

@MarcCote
Copy link
Contributor Author

Anything else @effigies ?

@effigies
Copy link
Member

Oh, yeah. Sorry. LGTM.

@matthew-brett
Copy link
Member

Thanks everyone for the input - merging.

@matthew-brett matthew-brett merged commit 01d3a92 into nipy:master Jun 27, 2016
@MarcCote MarcCote deleted the fix_tractogram_slicing branch June 27, 2016 18:59
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.

5 participants