Skip to content

Conversation

effigies
Copy link
Member

Suggested in #2598. Will allow us to test more vanilla installations of Python.

Should speed up Travis builds, too.

@effigies effigies added this to the 1.1.0 milestone May 27, 2018
@effigies
Copy link
Member Author

The failing test is related to the new numpy formatting.

>>> print(np.loadtxt(res.outputs.transform))
[[1. 0. 0. 0.]
[0. 1. 0. 0.]
[0. 0. 1. 0.]
[0. 0. 0. 1.]]
>>> reorient.inputs.orientation = 'RAS'
>>> res = reorient.run()
>>> res.outputs.out_file # doctest: +ELLIPSIS
'.../segmentation0_ras.nii.gz'
>>> print(np.loadtxt(res.outputs.transform))
[[-1. 0. 0. 60.]
[ 0. -1. 0. 72.]
[ 0. 0. 1. 0.]
[ 0. 0. 0. 1.]]

I'm not really sure why this wouldn't have appeared under conda, but there we are.

I guess the question is whether to change the test or update print options like so.

@satra
Copy link
Member

satra commented May 29, 2018

perhaps we can do something through the pytest config, just like we are doing doctests? we could also update the test to use our own format without relying on numpy.

@codecov-io
Copy link

codecov-io commented May 29, 2018

Codecov Report

Merging #2600 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2600      +/-   ##
==========================================
+ Coverage   67.62%   67.62%   +<.01%     
==========================================
  Files         339      339              
  Lines       42787    42787              
  Branches     5288     5288              
==========================================
+ Hits        28934    28936       +2     
+ Misses      13171    13168       -3     
- Partials      682      683       +1
Flag Coverage Δ
#smoketests 50.76% <ø> (ø) ⬆️
#unittests 65.09% <ø> (+0.01%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/dcm2nii.py 48.13% <ø> (ø) ⬆️
nipype/interfaces/niftyreg/regutils.py 80.86% <ø> (ø) ⬆️
nipype/interfaces/io.py 54.47% <ø> (ø) ⬆️
nipype/interfaces/base/core.py 89.7% <ø> (ø) ⬆️
nipype/interfaces/image.py 90.19% <ø> (ø) ⬆️
nipype/interfaces/dipy/setup.py 25% <0%> (+25%) ⬆️

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 205ea17...2025a06. Read the comment docs.

@effigies effigies force-pushed the ci/travis_python branch from 7cd2137 to 1ea55de Compare May 30, 2018 13:49
@effigies
Copy link
Member Author

we could also update the test to use our own format without relying on numpy.

Yeah. Apparently the details of the effective formatting change vary by what version of Python the wheel was compiled for (while conda packages seemed reasonably consistent). Added a print function that replaces the square brackets with spaces and letting the whitespace normalization do the rest.

@effigies
Copy link
Member Author

Passing Travis.

@effigies
Copy link
Member Author

@effigies
Copy link
Member Author

It's an indentation effect. .. testsetup:: and .. testcleanup:: are not treated correctly unless they start on the first character of the line. However, if we de-indent these, then the whole text of the docstring is treated as indented, changing appearance.

Is it acceptable to have a de-indented docstring?

@satra
Copy link
Member

satra commented Jun 5, 2018

@effigies - sorry didn't see this earlier. if it's necessary, it's necessary. looks darn ugly though :(

@effigies
Copy link
Member Author

effigies commented Jun 5, 2018

Yeah. Could be possible to fix up the Sphinx extension, but that's beyond my current ambitions.

@effigies effigies force-pushed the ci/travis_python branch from 152a444 to eca32f6 Compare June 5, 2018 13:51
@effigies
Copy link
Member Author

effigies commented Jun 5, 2018

Actually, I think all but one testsetup block can be removed.

@effigies effigies merged commit 704b97d into nipy:master Jun 18, 2018
@effigies effigies deleted the ci/travis_python branch June 18, 2018 01:39
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.

3 participants