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

Eigenvalue - eigenvector array compatibility check #1526

Merged
merged 5 commits into from Sep 2, 2018

Conversation

Projects
None yet
7 participants
@albayenes
Copy link
Contributor

albayenes commented May 19, 2018

If eigenvalues and aigenvector array shapes are not compatible, tensor_slicer actor does not give clear error message. In this PR, compatibility cheking of eigenvalue and eigenvector shapes is added. It gives more clear error when shapes are not compatible.

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented May 19, 2018

Hello @albayenes, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on September 02, 2018 at 17:09 Hours UTC
@arokem
Copy link
Member

arokem left a comment

Good addition!

I would probably make this a RuntimeError, so that it differentiates from the error that would be raised later on in the code in case inputs are incompatible.

Then, you could also add a test that checks that this error is raised when inputs are mis-matched.

e_s += "{0} for eigenvectors with".format(evals.shape)
e_s += "shape {0}. Please provide an eigenvector array".format(evecs.shape)
e_s += " that compatible with the eigenvalues array."
raise ValueError(e_s)

This comment has been minimized.

@arokem

arokem May 19, 2018

Member

I would suggest a small change in wording:

=> "Please provide eigenvector and eigenvalue arrays that have compatible dimensions"

@arokem

This comment has been minimized.

Copy link
Member

arokem commented May 19, 2018

I think that the CI failure is unrelated to this change. See #1527

@albayenes

This comment has been minimized.

Copy link
Contributor

albayenes commented May 19, 2018

Thank you for your review @arokem . I added your suggestions. I hope the test is convenient and enough. Should I worry about PEP8's complaints?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented May 19, 2018

The test is perfect. Please do fix up PEP8, by breaking up the long lines in the test.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented May 21, 2018

Looks good to me. We still need to deal with #1527, so we can see that the test runs properly.

e_s += " shape {0}. Please provide".format(evecs.shape)
e_s += " eigenvector and eigenvalue arrays"
e_s += " that have compatible dimensions."
raise RuntimeError(e_s)

This comment has been minimized.

@nilgoyette

nilgoyette May 22, 2018

Contributor

I think you should rewrite this like

raise RuntimeError(
    "You provided an eigenvalues array with a shape {} for "
    "eigenvectors with shape {}. Please provide eigenvector and "
    "eigenvalue arrays that have compatible dimensions."
    .format(evals.shape, evecs.shape))

to avoid a e_s variable (what's e_s?) and to have a more readable string with more than 3 words on a line! Also, the message sounds strange to my [french] ears. Maybe something simpler? "Eigenvalues shape {} is incompatible with eigenvectors' {}"? As you wish.

This comment has been minimized.

@albayenes

albayenes May 23, 2018

Contributor

Thanks for your review @nilgoyette . e_s stands for error string and it is widely used in dipy. If community thinks that your suggestions are better, I can fix it according to your suggestions.

This comment has been minimized.

@Garyfallidis

Garyfallidis May 28, 2018

Member

Both options are correct. There is no problem with using a string variable as input to RuntimeError Use either @albayenes these are both valid pythonic options. However, @nilgoyette 's suggestion to make the message more compact makes sense to me. So, I would shorten the message.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jun 12, 2018

Hey @albayenes : could you please rebase this one? I believe tests should pass now that #1542 is merged and with the availability of cvxpy wheels.

@albayenes albayenes force-pushed the albayenes:tensor-slicer-evals-evecs-shape-check branch from cf8202e to 80a06a4 Jun 12, 2018

@albayenes

This comment has been minimized.

Copy link
Contributor

albayenes commented Jun 12, 2018

@arokem, I rebased it.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Aug 22, 2018

Hi @albayenes, Could you rebase this one?

Sorry for this trouble, Travis is passing now, I think your PR is ready to go after your rebase.

Thank you

@albayenes albayenes force-pushed the albayenes:tensor-slicer-evals-evecs-shape-check branch from 80a06a4 to 1b19309 Sep 2, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 2, 2018

Codecov Report

Merging #1526 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1526      +/-   ##
==========================================
+ Coverage   87.33%   87.33%   +<.01%     
==========================================
  Files         246      246              
  Lines       32171    32177       +6     
  Branches     3494     3495       +1     
==========================================
+ Hits        28095    28102       +7     
  Misses       3242     3242              
+ Partials      834      833       -1
Impacted Files Coverage Δ
dipy/viz/actor.py 82.78% <100%> (+0.06%) ⬆️
dipy/viz/tests/test_actors.py 73.89% <100%> (+0.22%) ⬆️
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️

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 b6392e9...1b19309. Read the comment docs.

@albayenes

This comment has been minimized.

Copy link
Contributor

albayenes commented Sep 2, 2018

@skoudoro sorry for late rebase. I overlooked your request.

@skoudoro skoudoro merged commit 4890903 into nipy:master Sep 2, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.33%)
Details
codecov/project 87.33% (+<.01%) compared to b6392e9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 2, 2018

No worries, Thank you @albayenes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment