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

[WIP] Switching tests to pytest and removing nose dependencies #1414

Closed
wants to merge 58 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@thechargedneutron
Copy link
Contributor

thechargedneutron commented Feb 4, 2018

Fixes #1280

Please review if this is what you intended. I have added separate check function and used it.

thechargedneutron added some commits Feb 2, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 6, 2018

Codecov Report

Merging #1414 into master will decrease coverage by 0.53%.
The diff coverage is 62.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1414      +/-   ##
==========================================
- Coverage   87.55%   87.01%   -0.54%     
==========================================
  Files         246      240       -6     
  Lines       31366    30624     -742     
  Branches     3433     3121     -312     
==========================================
- Hits        27464    26649     -815     
- Misses       3085     3179      +94     
+ Partials      817      796      -21
Impacted Files Coverage Δ
dipy/tests/test_scripts.py 100% <ø> (ø) ⬆️
dipy/reconst/tests/test_peak_finding.py 96.8% <0%> (ø) ⬆️
dipy/testing/tests/test_decorators.py 95.65% <100%> (ø) ⬆️
dipy/tracking/tests/test_metrics.py 100% <100%> (ø) ⬆️
dipy/workflows/tests/test_masking.py 91.66% <100%> (ø) ⬆️
dipy/segment/tests/test_quickbundles.py 98.31% <100%> (+0.02%) ⬆️
dipy/tracking/tests/test_track_volumes.py 100% <100%> (ø) ⬆️
dipy/workflows/tests/test_tracking.py 95.12% <100%> (ø) ⬆️
dipy/tracking/tests/test_learning.py 100% <100%> (ø) ⬆️
dipy/reconst/tests/test_fwdti.py 100% <100%> (ø) ⬆️
... and 115 more

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 327af13...7155589. Read the comment docs.

thechargedneutron added some commits Feb 7, 2018

@thechargedneutron

This comment has been minimized.

Copy link
Contributor

thechargedneutron commented Feb 21, 2018

@skoudoro Can you please have a look at this PR and suggest what changes should be done?

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @thechargedneutron, Thank you for starting this change. Here my first comment:

  • Can you rebase this PR.
  • Can you remove all *.html files and .cache files.
  • Can you explain to me what is ashu, ashurc and ashutests?
  • This line should be replace by pytest --doctest-modules --durations=10 -svv dipy
  • find coverage option with pytest

When this first part is good, you can start to look at:

  • pytest and coverage (without plugin)
  • pytest and benchmark(without plugin)
  • skiptest
"dipy/workflows/tests/test_reconst_dti.py": true,
"dipy/workflows/tests/test_segment.py": true,
"dipy/workflows/tests/test_workflow.py": true
}

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

This file should be removed

.travis.yml Outdated
@@ -86,7 +86,8 @@ before_install:
- python --version # just to check
# Needed for Python 3.5 wheel fetching
- $PIPI --upgrade pip setuptools
- $PIPI nose;
- $PIPI nose

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

we do not want nose anymore so you can remove this line

@@ -7,13 +7,13 @@
import dipy.reconst as dire
dire.bench()
If you have doctests enabled by default in nose (with a noserc file or
If you have doctests enabled by default in ashu (with a ashurc file or

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

what is ashu ?

environment variable), and you have a numpy version <= 1.6.1, this will also
run the doctests, let's hope they pass.
Run this benchmark with:
nosetests -s --match '(?:^|[\\b_\\.//-])[Bb]ench' /path/to/bench_bounding_box.py
ashutests -s --match '(?:^|[\\b_\\.//-])[Bb]ench' /path/to/bench_bounding_box.py

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

what is ashutests ?

@@ -5,13 +5,13 @@
import dipy.reconst as dire
dire.bench()
If you have doctests enabled by default in nose (with a noserc file or
If you have doctests enabled by default in ashu (with a ashurc file or

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

same comment as below

environment variable), and you have a numpy version <= 1.6.1, this will also
run the doctests, let's hope they pass.
Run this benchmark with:
nosetests -s --match '(?:^|[\\b_\\.//-])[Bb]ench' /path/to/bench_squash.py
ashutests -s --match '(?:^|[\\b_\\.//-])[Bb]ench' /path/to/bench_squash.py

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

same comment as above

@@ -121,7 +121,7 @@ def old_squash(arr, mask=None, fill=0):


def bench_quick_squash():
# nosetests -s --match '(?:^|[\\b_\\.//-])[Bb]ench'
# ashutests -s --match '(?:^|[\\b_\\.//-])[Bb]ench'

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

same comment as above

@@ -5,7 +5,7 @@
import dipy.reconst as dire
dire.bench()
If you have doctests enabled by default in nose (with a noserc file or
If you have doctests enabled by default in ashu (with a ashurc file or

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

same comment as above

@@ -26,7 +26,7 @@ def with_einsum(f): return f

@with_einsum
def bench_vec_val_vect():
# nosetests -s --match '(?:^|[\\b_\\.//-])[Bb]ench'
# ashutests -s --match '(?:^|[\\b_\\.//-])[Bb]ench'

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

same comment as above

@@ -0,0 +1,4329 @@
<!DOCTYPE html>

This comment has been minimized.

@skoudoro

skoudoro Mar 3, 2018

Member

file to remove

@thechargedneutron

This comment has been minimized.

Copy link
Contributor

thechargedneutron commented Mar 5, 2018

@skoudoro Sorry for the sloppy work in this PR. ashu was only used to mask the occurrences of the word nose. Really sorry for this.
Also the .cache and .html files are removed. But I don't why it was created.

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @thechargedneutron, here some comment to improve the tests. As you can see here, pytest require the parameter reason to be set up. So you need to update it like this :
@pytest.mark.skipif(not SCIPY_LESS_0_12, reason="requires scipy < 0.12")

Approximately 30 functions need this update.

@@ -57,7 +58,7 @@ def test_optimize_new_scipy():
npt.assert_array_almost_equal(opt.xopt, np.array([0, 0, 0, 0.]))


@npt.dec.skipif(not SCIPY_LESS_0_12)
@pytest.mark.skipif(not SCIPY_LESS_0_12)

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member

As you can see here, pytest need the parameter reason. so you need to update it like this :
@pytest.mark.skipif(not SCIPY_LESS_0_12, reason="requires scipy < 0.12")

assert np.all(actual == approx(desired))

def assert_array_almost_equal(actual, desired, decimal=7):
assert np.all(actual == approx(desired, rel=10**(-1*decimal)))

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member
  1. Like you did before, can you add try..... except AssertionError..... for getting more information when there is a problem
  2. I think np.all is not necessary. you can just do assert actual == approx(desired, rel=10**(-1*decimal))

This comment has been minimized.

@skoudoro

skoudoro Apr 3, 2018

Member

Hi @thechargedneutron, I think you are close, Any chance to make this change?

assert np.all(actual == approx(desired, rel=10**(-1*decimal)))

def assert_(statement):
assert statement

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member

Like you did before, can you add try..... except AssertionError..... for getting more information when there is a problem

assert statement

def assert_array_less(first, second):
assert np.all(first < second)

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member

Like you did before, can you add try..... except AssertionError..... for getting more information when there is a problem

def compare(x, y):
return np.core.numeric.isclose(x, y, rtol=rtol, atol=atol, equal_nan=equal_nan)

actual, desired = np.asanyarray(actual), np.asanyarray(desired)

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member

Like you did before, can you add try..... except AssertionError..... for getting more information when there is a problem

raise AssertionError("Values are not almost equal to {} decimals".format(decimal))

def assert_array_equal(actual, desired):
assert np.all(actual == approx(desired))

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member
  1. Like you did before, can you add try..... except AssertionError..... for getting more information when there is a problem
  2. I think np.all is not necessary. you can just do assert actual == desired

def assert_almost_equal(actual, desired, decimal=7):
if not abs(desired - actual) < 1.5 * 10**(-decimal):
raise AssertionError("Values are not almost equal to {} decimals".format(decimal))

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member
  1. Like you did before, can you add try..... except AssertionError..... for getting more information when there is a problem
@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Mar 22, 2018

[31m�[1m= 118 failed, 598 passed, 18 skipped, 63 warnings, 43 error in 1109.22 seconds

80% passed tests -> you are close :-)

@thechargedneutron

This comment has been minimized.

Copy link
Contributor

thechargedneutron commented May 6, 2018

@skoudoro Can you review this again? A lot of doctest are failing. I can't figure out the reason for this. Also, a few instances of assert_equal and assert_array_equal are failing too. I could not figure out the reasons for it.

thechargedneutron added some commits May 6, 2018

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @thechargedneutron, few remarks:

  • Can you apply my previous suggestion? try..... except AssertionError..... for getting more information from all your assert_* function
  • Your conversion is not needed. I do not understand why you added np.all and np.asarray everywhere. I think it is not necessary and your first version was better.
  • Concerning doctest, you have to setup numpy print option (set_printoptions numpy). This is the reason of your error. I recommend you to keep this point as the last focus.
if not 'numpy' in str(type(value1)):
value1 = np.array(value1)
if not 'numpy' in str(type(value2)):
value2 = np.array(value2)

This comment has been minimized.

@skoudoro

skoudoro May 6, 2018

Member

Why are you doing this test?

This comment has been minimized.

@thechargedneutron

thechargedneutron May 6, 2018

Contributor

There were several instances where value1 = [np.array([5,6])] and value2 = [np.array([5,6])]. This is a list and checking using == yields error. Any simple work around for this?

This comment has been minimized.

@skoudoro

skoudoro May 6, 2018

Member

as you can see here, pytest manage numpy array and list of numpy array, so I am really not sure we need this test

if not 'numpy' in str(type(value2)):
value2 = np.array(value2)
try:
assert np.all(value1 == value2)

This comment has been minimized.

@skoudoro

skoudoro May 6, 2018

Member

why are you using np.all? Not needed I think

This comment has been minimized.

@thechargedneutron

thechargedneutron May 6, 2018

Contributor

There are some numpy arrays checked using assert_equal. Hence needed to use np.all.

This comment has been minimized.

@skoudoro

skoudoro May 6, 2018

Member

as you can see here, pytest manage numpy array and list of numpy array, so I still think that np.all is not needed

if not 'numpy' in str(type(value1)):
value1 = np.array(value1)
if not 'numpy' in str(type(value2)):
value2 = np.array(value2)

This comment has been minimized.

@skoudoro

skoudoro May 6, 2018

Member

same comment as below

thechargedneutron added some commits May 7, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Oct 16, 2018

superseded by #1652. Closing

@skoudoro skoudoro closed this Oct 16, 2018

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