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

Projects
None yet
5 participants
@MarcCote
Contributor

MarcCote commented Apr 19, 2017

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

@codecov-io

This comment has been minimized.

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.

RF: finish removing extern stdlib imports
Use calloc etc from libc.stdlib, and memset, memcpy from libc.string.
@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 19, 2017

Finishing up stdlib import removals at MarcCote#10 .

Merge pull request #10 from matthew-brett/remove-stdlib-externs
RF: finish removing extern stdlib imports
@coveralls

This comment has been minimized.

coveralls commented Apr 19, 2017

Coverage Status

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

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Apr 20, 2017

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

@arokem arokem merged commit 81a3a77 into nipy:master Apr 22, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 568d14c...287d30e
Details
codecov/project 85.88% remains the same compared to 568d14c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 88.391%
Details
@arokem

This comment has been minimized.

Member

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

This comment has been minimized.

Member

matthew-brett commented Apr 23, 2017

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

This comment has been minimized.

Member

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 MarcCote:rf_use_libc_stdlib branch Oct 5, 2017

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1228 from MarcCote/rf_use_libc_stdlib
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