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

BUG: Fix factorial import module in test_mapmri.py. #1326

Merged
merged 1 commit into from Jan 12, 2018

Conversation

Projects
None yet
5 participants
@jhlegarreta
Contributor

jhlegarreta commented Sep 9, 2017

Fix the factorial function import module in test_mapmri.py. The function
was duplicated from the scipy.misc module to scipy.special in SciPy
release 0.14.0:
https://docs.scipy.org/doc/scipy-0.14.0/reference/special.html
vs.
https://docs.scipy.org/doc/scipy-0.13.0/reference/special.html
and
https://docs.scipy.org/doc/scipy-0.13.0/reference/misc.html

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Sep 9, 2017

Addresses some of the warnings in latest builds.

@coveralls

This comment has been minimized.

coveralls commented Sep 9, 2017

Coverage Status

Coverage decreased (-0.007%) to 85.328% when pulling 57a4f22 on jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning into e498e68 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 9, 2017

Coverage Status

Coverage decreased (-0.007%) to 85.328% when pulling 57a4f22 on jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning into e498e68 on nipy:master.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch from 57a4f22 to e58a602 Sep 20, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Sep 20, 2017

Codecov Report

Merging #1326 into master will decrease coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1326      +/-   ##
==========================================
- Coverage   87.07%   87.07%   -0.01%     
==========================================
  Files         227      227              
  Lines       29063    29066       +3     
  Branches     3125     3125              
==========================================
+ Hits        25307    25309       +2     
- Misses       3046     3047       +1     
  Partials      710      710
Impacted Files Coverage Δ
dipy/reconst/tests/test_mapmri.py 98.56% <100%> (ø) ⬆️
dipy/reconst/mapmri.py 89.3% <81.81%> (-0.09%) ⬇️

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 e0db0d7...7aacdc3. Read the comment docs.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch from e58a602 to 05c8b75 Oct 19, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Oct 19, 2017

Rebased 05c8b75 onto master to see if the errors go away. Looks weird that the codecov does not pass, since these are only import fixes and travis reported no failure.

@arokem

This comment has been minimized.

Member

arokem commented Oct 25, 2017

Could you pleas try rebasing again?

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch from 05c8b75 to 94a6a66 Oct 26, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Oct 26, 2017

Sure. Done in 94a6a66 .

@skoudoro

This comment has been minimized.

Member

skoudoro commented Nov 22, 2017

Hi @jhlegarreta! What is the difference between from math import factorial and from scipy.special import factorial ? Is there any performance issue?

If no, It would be great if you replace this try/except with the math standard library. What do you think ?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Nov 22, 2017

Can you replace it on dipy/reconst/mapmri.py file too ? Thanks !

@arokem

This comment has been minimized.

Member

arokem commented Jan 2, 2018

Yeah - since it's used here on an integer (rather than an array of integers), it seems that it would be advantageous to use the math.factorial function rather than the scipy version:

In [1]: from math import factorial as mfactorial

In [2]: from scipy.special import factorial as sfactorial

In [3]: %timeit mfactorial(10)
163 ns ± 5.12 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [4]: %timeit sfactorial(10)
6.61 µs ± 51.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)


@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch from 94a6a66 to daaf322 Jan 5, 2018

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Jan 5, 2018

Done. I think I was careful to choose between the math.factorial, and scipy.special.factorial versions, but let's see what CI says. Also took advantage to rebase on top of master.

@@ -29,7 +29,7 @@ def int_func(n):
f = np.sqrt(2) * factorial(n) / float(((gamma(1 + n / 2.0)) *
np.sqrt(2**(n + 1) * factorial(n))))
return f
s

This comment has been minimized.

@skoudoro

skoudoro Jan 6, 2018

Member

small typo mistake here and travis does not like it ;-)

Otherwise, Thanks for this update !

This comment has been minimized.

@jhlegarreta

jhlegarreta Jan 6, 2018

Contributor

Fixed. Thanks for the reveiw @skoudoro. Let's see what travis says about d9a44d2

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch from daaf322 to d9a44d2 Jan 6, 2018

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Jan 8, 2018

Well, the only difference I see is that the int_func function now uses the math.factorial method instead of scipy.misc.factorial.

Since I cannot debug, I am unable to propose a solution.

I am up-to-date with master, and I get the error

Traceback (most recent call last):
  File "C:\Program Files (x86)\JetBrains\PyCharm Community Edition 2016.3.2\helpers\pydev\pydevd.py", line 1596, in <module>
    globals = debugger.run(setup['file'], None, None, is_module)
  File "C:\Program Files (x86)\JetBrains\PyCharm Community Edition 2016.3.2\helpers\pydev\pydevd.py", line 974, in run
    pydev_imports.execfile(file, globals, locals)  # execute the script
  File "C:/SDKs/dipy/dipy/reconst/tests/test_mapmri.py", line 20, in <module>
    from dipy.direction.peaks import peak_directions
  File "C:\Program Files (x86)\JetBrains\PyCharm Community Edition 2016.3.2\helpers\pydev\_pydev_bundle\pydev_monkey_qt.py", line 71, in patched_import
    return original_import(name, *args, **kwargs)
  File "C:\SDKs\dipy\dipy\direction\__init__.py", line 2, in <module>
    from .probabilistic_direction_getter import ProbabilisticDirectionGetter
  File "C:\Program Files (x86)\JetBrains\PyCharm Community Edition 2016.3.2\helpers\pydev\_pydev_bundle\pydev_monkey_qt.py", line 71, in patched_import
    return original_import(name, *args, **kwargs)
ImportError: No module named probabilistic_direction_getter

When I open dipy\dipy\direction\__init__.py, PyCharm seems to warn me about unresolved reference to probabilistic_direction_getter. I've seen that CI on master is builing OK, so the problem seems to be local. Any ideas where this comes from so that I can debug and see where travis is finding difficulties?

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch from d9a44d2 to 8c93758 Jan 8, 2018

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Jan 8, 2018

Although my local issues persist, giving it a try with scipy.misc.factorial where travis located the error in case it is an issue of having an array rather than a scalar. Let's what travis says for 8c93758

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch from 8c93758 to c62feca Jan 8, 2018

@arokem

This comment has been minimized.

Member

arokem commented Jan 8, 2018

The errors now on Python 3 seem unrelated to your changes. Might be something else? Is this currently also happening on master?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Jan 8, 2018

I just looked at it and it seems to appear on other PR like #1384 @arokem. Something change on python3 or numpy?

Jon Haitz Legarreta Gorroño
BUG: Fix factorial import module in test_mapmri.py.
Fix the factorial function import module in test_mapmri.py. The function
was duplicated from the scipy.misc module to scipy.special in SciPy
release 0.14.0:
https://docs.scipy.org/doc/scipy-0.14.0/reference/special.html
vs.
https://docs.scipy.org/doc/scipy-0.13.0/reference/special.html
and
https://docs.scipy.org/doc/scipy-0.13.0/reference/misc.html

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch from c62feca to 7aacdc3 Jan 12, 2018

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Jan 12, 2018

Rebased on master after #1399 was closed. Let's see what this gives. Thanks @arokem @skoudoro for having looked into the CI issues.

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Jan 12, 2018

Travis is happy !

@arokem

This comment has been minimized.

Member

arokem commented Jan 12, 2018

Hey @jhlegarreta - regarding your cython problems: have you tried rebuilding the extensions locally? Something like:

python setup.py build_ext --inplace 
@@ -1541,7 +1544,7 @@ def mapmri_isotropic_K_mu_dependent(radial_order, mu, rgrad):
def binomialfloat(n, k):
"""Custom Binomial function
"""
return factorial(n) / (factorial(n - k) * factorial(k))
return sfactorial(n) / (sfactorial(n - k) * sfactorial(k))

This comment has been minimized.

@jhlegarreta

jhlegarreta Jan 12, 2018

Contributor

Since I seem to be unable to debug the file due to some error probably linked to the dipy/direction/probabilistic_direction_getter.pyx file (may be some Cython-related error), I'm not that sure whether mfactorial would be "enough" here (i.e. integers not arrays).

@arokem

This comment has been minimized.

Member

arokem commented Jan 12, 2018

For the time being, this looks fine to me, so I will go ahead and merge. If you want to make subsequent changes, feel free to continue this on another PR. Thanks!

@arokem arokem merged commit b3068e6 into nipy:master Jan 12, 2018

1 of 3 checks passed

codecov/patch 83.33% of diff hit (target 87.07%)
Details
codecov/project 87.07% (-0.01%) compared to e0db0d7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Jan 12, 2018

@arokem Thanks for the suggestion. Well, old problems are back in Win: error: Unable to find vcvarsall.bat rebuilding the extensions... Need to have a look at it.

Thanks for merging.

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

Merge pull request nipy#1326 from jhlegarreta/AddressScipyFactorialMi…
…scModuleDeprecationWarning

BUG: Fix factorial import module in test_mapmri.py.

@jhlegarreta jhlegarreta deleted the jhlegarreta:AddressScipyFactorialMiscModuleDeprecationWarning branch Oct 10, 2018

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