Skip to content
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: Use cython imports instead of relying on extern #1228

Merged
merged 3 commits into from Apr 22, 2017

Conversation

MarcCote
Copy link
Contributor

Small refactoring as suggested by @matthew-brett (see comment #1076 (comment))

@codecov-io
Copy link

codecov-io commented Apr 19, 2017

Codecov Report

Merging #1228 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1228   +/-   ##
=======================================
  Coverage   85.88%   85.88%           
=======================================
  Files         221      221           
  Lines       27285    27285           
  Branches     2785     2785           
=======================================
  Hits        23435    23435           
  Misses       3165     3165           
  Partials      685      685

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 568d14c...287d30e. Read the comment docs.

Use calloc etc from libc.stdlib, and memset, memcpy from libc.string.
@matthew-brett
Copy link
Contributor

Finishing up stdlib import removals at MarcCote#10 .

RF: finish removing extern stdlib imports
@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.391% when pulling 287d30e on MarcCote:rf_use_libc_stdlib into 568d14c on nipy:master.

@MarcCote
Copy link
Contributor Author

MarcCote commented Apr 20, 2017

@matthew-brett thanks. @arokem this PR is ready.

@arokem arokem merged commit 81a3a77 into dipy:master Apr 22, 2017
@arokem
Copy link
Contributor

arokem commented Apr 22, 2017

Darn. Should've probably asked you to rebase on master before merging. Looks like this broke the master branch: https://travis-ci.org/nipy/dipy/jobs/224600749. Any idea how these things interact?

@matthew-brett
Copy link
Contributor

Error is:

======================================================================
FAIL: dipy.reconst.tests.test_dki_micro.test_single_fiber_model
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/nipy/dipy/venv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/nipy/dipy/venv/lib/python2.7/site-packages/dipy/reconst/tests/test_dki_micro.py", line 131, in test_single_fiber_model
    assert_almost_equal(wmtiF.tortuosity, ADe/RDe)
  File "/home/travis/build/nipy/dipy/venv/lib/python2.7/site-packages/numpy/testing/utils.py", line 468, in assert_almost_equal
    raise AssertionError(msg)
AssertionError: 
Arrays are not almost equal to 7 decimals
 ACTUAL: 2.59770124494259
 DESIRED: 2.597701149425287

I immediately can't think of any way the stdlib imports could cause a change at at the 7th decimal place, only for Cython 0.25.1 / numpy 1.7.1. Maybe there's some randomness in the test?

@arokem
Copy link
Contributor

arokem commented Apr 23, 2017

You're right. This is also relatively new, so might just be the precision wasn't set appropriately: #1231

To answer your question: yes, there is some randomness in that test: https://github.com/nipy/dipy/blob/master/dipy/reconst/tests/test_dki_micro.py#L88-L89

@MarcCote MarcCote deleted the rf_use_libc_stdlib branch October 5, 2017 11:44
ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018
RF: Use cython imports instead of relying on extern
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.

None yet

5 participants