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

repr and get methods for AffineMap, w/ precise exceptions #1445

Merged
merged 6 commits into from Mar 12, 2018

Conversation

Projects
None yet
3 participants
@raamana
Contributor

raamana commented Mar 5, 2018

PR from discussion in #1442

Changes pass the tests in dipy.align/tests/test_imaffine.py

Still need to discuss

  1. whether we should remove affine.affine, and make it private
  2. the alternative: @Property and @property.setter etc
@raamana

This comment has been minimized.

Contributor

raamana commented Mar 5, 2018

Usage is illustrated below:

from dipy.align.imaffine import AffineMap
import numpy as np

init = np.random.random([4, 4])
a = AffineMap(init)

a

array([[0.87399387, 0.36447063, 0.49235451, 0.24511999],
       [0.37959061, 0.33566857, 0.84241025, 0.49286616],
       [0.92397491, 0.67201834, 0.78806139, 0.95720898],
       [0.46861731, 0.12886398, 0.38019741, 0.83287818]])

print(a)

[[0.87399387 0.36447063 0.49235451 0.24511999]
 [0.37959061 0.33566857 0.84241025 0.49286616]
 [0.92397491 0.67201834 0.78806139 0.95720898]
 [0.46861731 0.12886398 0.38019741 0.83287818]]

print('Full:\n{a:full}\nRotation:\n{a:rotation}\nTranslation:\n{a:translation}'.format(a=a))

Full:
[[0.87399387 0.36447063 0.49235451 0.24511999]
 [0.37959061 0.33566857 0.84241025 0.49286616]
 [0.92397491 0.67201834 0.78806139 0.95720898]
 [0.46861731 0.12886398 0.38019741 0.83287818]]
Rotation:
[[0.87399387 0.36447063 0.49235451]
 [0.37959061 0.33566857 0.84241025]
 [0.92397491 0.67201834 0.78806139]]
Translation:
[[0.24511999]
 [0.49286616]
 [0.95720898]]

print('Full:\n{a:f}\nRotation:\n{a:r}\nTranslation:\n{a:t}'.format(a=a))

Full:
[[0.87399387 0.36447063 0.49235451 0.24511999]
 [0.37959061 0.33566857 0.84241025 0.49286616]
 [0.92397491 0.67201834 0.78806139 0.95720898]
 [0.46861731 0.12886398 0.38019741 0.83287818]]
Rotation:
[[0.87399387 0.36447063 0.49235451]
 [0.37959061 0.33566857 0.84241025]
 [0.92397491 0.67201834 0.78806139]]
Translation:
[[0.24511999]
 [0.49286616]
 [0.95720898]]

Setter throws errors invalid input or elements or non-invertible matrices:

a.set_affine(1)

Traceback (most recent call last):
  File "/home/praamana/anaconda2/envs/py36/lib/python3.6/site-packages/IPython/core/interactiveshell.py", line 2881, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-12-f67de536707e>", line 1, in <module>
    a.set_affine(1)
  File "/home/praamana/dev/dipy/dipy/align/imaffine.py", line 171, in set_affine
    raise TypeError('Input is not of type ndarray.')
TypeError: Input is not of type ndarray.


init[1,2] = np.nan
a.set_affine(init)

Traceback (most recent call last):
  File "/home/praamana/anaconda2/envs/py36/lib/python3.6/site-packages/IPython/core/interactiveshell.py", line 2881, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-14-3e7f2be45bd5>", line 1, in <module>
    a.set_affine(init)
  File "/home/praamana/dev/dipy/dipy/align/imaffine.py", line 174, in set_affine
    raise AffineInvalidValuesError('Affine contains invalid elements')
dipy.align.imaffine.AffineInvalidValuesError: Affine contains invalid elements
@codecov-io

This comment has been minimized.

codecov-io commented Mar 5, 2018

Codecov Report

Merging #1445 into master will increase coverage by <.01%.
The diff coverage is 91.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1445      +/-   ##
==========================================
+ Coverage   87.42%   87.43%   +<.01%     
==========================================
  Files         239      239              
  Lines       30579    30640      +61     
  Branches     3291     3307      +16     
==========================================
+ Hits        26735    26789      +54     
- Misses       3075     3079       +4     
- Partials      769      772       +3
Impacted Files Coverage Δ
dipy/align/tests/test_imaffine.py 99.39% <100%> (+0.05%) ⬆️
dipy/align/imaffine.py 91.84% <86.36%> (-0.95%) ⬇️
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️

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 6a78d50...1f09993. Read the comment docs.

@skoudoro

Thanks @raamana for this PR. I just made a couple of pep8 issues.

Otherwise, I vote for

remove affine.affine, and make it private
use the alternative: @Property and @property.setter

It will be good to have other opinions

@@ -67,6 +67,9 @@
class AffineInversionError(Exception):
pass
class AffineInvalidValuesError(Exception):

This comment has been minimized.

@skoudoro

skoudoro Mar 5, 2018

Member

pep8: expected 2 blank lines

return str(self.affine)
# rotation part only (initial 3x3)
elif format_spec in ['r', 'rotation']:
return str(self.affine[:-1,:-1])

This comment has been minimized.

@skoudoro

skoudoro Mar 5, 2018

Member

pep8: missing space after ,

elif format_spec in ['t', 'translation']:
# notice unusual indexing to make it a column vector
# i.e. rows from 0 to n-1, cols from n to n
return str(self.affine[:-1,-1:])

This comment has been minimized.

@skoudoro

skoudoro Mar 5, 2018

Member

pep8: missing space after ,

return str(self.affine)
def __repr__(self):
"""Reloable represenation - also relies on ndarray's implementation."""

This comment has been minimized.

@skoudoro

skoudoro Mar 5, 2018

Member

typo: representation

raise NotImplementedError('Format {} not recognized or implemented.\n'
'Try one of {}'.format(format_spec, allowed_formats_print_map))

This comment has been minimized.

@skoudoro

skoudoro Mar 5, 2018

Member

pep8: too many blank lines

This comment has been minimized.

@raamana

raamana Mar 6, 2018

Contributor

isn't two lines between two methods standard? Atleast my Pycharm thinks so

This comment has been minimized.

@skoudoro

skoudoro Mar 6, 2018

Member

The best answer is here: https://www.python.org/dev/peps/pep-0008/#blank-lines

Method definitions inside a class are surrounded by a single blank line.

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 5, 2018

Can you add some tests for __str__, __repr__, and __format__ ?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 6, 2018

Thank you for adding the tests! As you can see here and here, you can improve them to make Codecov happy :-)

@raamana

This comment has been minimized.

Contributor

raamana commented Mar 6, 2018

My bad, I didn't think about the test coverage :)

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 6, 2018

Great! Thanks @raamana!

We still need to make a decision:

whether we should remove affine.affine, and make it private
the alternative: @Property and @property.setter etc

Any opinion @arokem @matthew-brett @Garyfallidis @omarocegueda?

@raamana

This comment has been minimized.

Contributor

raamana commented Mar 8, 2018

You’re welcome.

Perhaps, we can merge this for now, and plan a update/deprecation cycle for modifying attribute behaviour, however it’s voted to be? This PR is backwards compatible, and wouldn’t require changes in users code.

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 9, 2018

Ok, I will merge this PR in 3days if there is no objection.

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 12, 2018

Thanks @raamana! merging!

@skoudoro skoudoro merged commit 401b2d3 into nipy:master Mar 12, 2018

3 checks passed

codecov/patch 91.78% of diff hit (target 87.42%)
Details
codecov/project 87.43% (+<.01%) compared to 6a78d50
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1445 from raamana/master
repr and get methods for AffineMap, w/ precise exceptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment