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

RF: make cvxopt optional for tests #363

Merged
merged 2 commits into from May 12, 2014

Conversation

Projects
None yet
2 participants
@matthew-brett
Member

matthew-brett commented May 11, 2014

Protect against errors in tests when cvxopt not installed.

RF: make cvxopt optional for tests
Protect against errorts in tests when cvxopt not installed.

@matthew-brett matthew-brett referenced this pull request May 11, 2014

Closed

cvxopt dependency #359

BF: fix funky error using __import__ on cvxopt
Very strange error importing cvxopt with python3 and __import__; maybe
this is because I was using an half-documented form of the function
(passing an empty string in `fromlist`).

Don't pass empty string when it's not needed in optpkg
from warnings import warn
from math import factorial

This comment has been minimized.

@arokem

arokem May 12, 2014

Member

Unrelated to this PR, but is this one actually better/faster than scipy.misc.factorial?

@arokem

This comment has been minimized.

Member

arokem commented May 12, 2014

Looks good to me

@arokem

This comment has been minimized.

Member

arokem commented May 12, 2014

Again, unrelated to this PR, here are a few things that are not tested in this code:

  1. This error: https://github.com/matthew-brett/dipy/blob/cvxopt-optional/dipy/reconst/shore.py#L169
  2. Calculating tau when it's not defined: https://github.com/matthew-brett/dipy/blob/cvxopt-optional/dipy/reconst/shore.py#L178
  3. This error: https://github.com/matthew-brett/dipy/blob/cvxopt-optional/dipy/reconst/shore.py#L180
  4. This one is tricky. What happens if no solution if found: https://github.com/matthew-brett/dipy/blob/cvxopt-optional/dipy/reconst/shore.py#L267
  5. This caching trick (the documentation there could also be better): https://github.com/matthew-brett/dipy/blob/cvxopt-optional/dipy/reconst/shore.py#L339
  6. This method: https://github.com/matthew-brett/dipy/blob/cvxopt-optional/dipy/reconst/shore.py#L461
  7. This error: https://github.com/matthew-brett/dipy/blob/cvxopt-optional/dipy/reconst/shore.py#L741
  8. This error: https://github.com/matthew-brett/dipy/blob/cvxopt-optional/dipy/reconst/shore.py#L778

I admit that most of these are rather trivial. Error handling is a no-brainer and I don't mean to pick on anyone. Just thought I would make a list of these things, so that it's easier to go back and improve testing, when we get a chance.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented May 12, 2014

Ariel - is it OK to open another issue for those things? I agree, it's good to keep track of problems as we see them.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented May 12, 2014

I'd add : can the code in the docstring in ShoreModel be made into a doctest?

@arokem

This comment has been minimized.

Member

arokem commented May 12, 2014

Yes and yes. I was just being lazy, but please do make this a new issue. I will merge your stuff in the meanwhile. Here goes

arokem added a commit that referenced this pull request May 12, 2014

Merge pull request #363 from matthew-brett/cvxopt-optional
RF: make cvxopt optional for tests

@arokem arokem merged commit 8abe71f into nipy:master May 12, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment