[MRG + 2] Fixes #4385: Move newton_cg test out of optimize #4389

vinayak-mehta · 2015-03-13T12:06:32Z

I had to make new grad and hess functions from func_grad_hess to resolve errors. Still, I'm getting an error with fmin_ncg:

Traceback (most recent call last):
  File "test.py", line 25, in 
    assert_almost_equal(newton_cg(func_grad_hess, func, grad, x0), fmin_ncg(f=func, x0=x0, fprime=grad, fhess=hess))
  File "/usr/lib/python3.4/site-packages/scipy/optimize/optimize.py", line 1313, in fmin_ncg
    callback=callback, **opts)
  File "/usr/lib/python3.4/site-packages/scipy/optimize/optimize.py", line 1405, in _minimize_newtoncg
    Ap = numpy.dot(A, psupi)
TypeError: unsupported operand type(s) for *: 'function' and 'float'

Can anyone help me to resolve this?
ping @amueller

UPD The assertion is holding for some values now :|
UPD I think the test works fine now.

landscape-bot · 2015-03-13T12:18:54Z

Repository health increased by 0.01% when pulling 5ad1365 on vortex-ape:move_test into 4a57c92 on scikit-learn:master.

No new problems were introduced.
1 problem was fixed (including 0 errors and 1 code smell).

amueller · 2015-03-16T19:44:11Z

sklearn/utils/tests/test_optimize.py

The whole should wrapped in a test_newton_cg function.

landscape-bot · 2015-03-17T22:26:18Z

Repository health increased by 0.01% when pulling aed7460 on vortex-ape:move_test into d69f2d6 on scikit-learn:master.

No new problems were introduced.
1 problem was fixed (including 0 errors and 1 code smell).

amueller · 2015-03-18T03:27:09Z

What do you mean with "Some values"? the test is passing, right? You mean it is only passing for certain random values? Btw, the tests should fix the seed, by doin

rng = np.random.RandomState(0)
X = rng.normal(...)

vinayak-mehta · 2015-03-18T15:19:33Z

I ran the file separately for a number of times. Yeah, it is passing for certain random values.

amueller · 2015-03-18T15:48:43Z

can you give an example that is failing?

vinayak-mehta · 2015-03-19T02:25:21Z

[[  1.15708586e+00  -7.53554218e-01   1.92638340e-01  -4.22095972e-01
    1.05947906e+00   7.12534115e-01  -1.08987771e+00  -8.67327002e-02
    8.91012678e-01  -6.49361597e-01]
 [  5.11517798e-01   5.42748472e-01  -1.70962041e+00   3.44213266e-01
   -6.10681256e-01   1.66858078e-01  -2.78617883e-01   2.21745662e-01
   -4.90909754e-01   3.36908877e-02]
 [ -1.11800422e+00  -9.53622890e-01   1.22303315e-01  -4.99311580e-01
   -1.35566242e+00   1.64765322e-01  -8.43358801e-01  -1.01145299e+00
   -1.40650492e+00  -1.40414658e+00]
 [  6.77320171e-01  -6.38708002e-02   2.52441936e+00   3.94445473e-01
   -6.46949685e-02  -1.09303154e+00   9.04073980e-01  -6.19499970e-01
    1.04766931e+00   1.29344126e+00]
 [  5.62310901e-01   9.73506318e-01  -1.50972147e+00  -1.01770862e+00
   -2.09378869e-01   1.49795408e+00  -3.96479915e-01  -3.19679964e-01
   -5.85308304e-03  -2.09192281e-01]
 [ -9.51038400e-01   9.70086713e-02   1.67750887e-01   2.18236953e-01
    8.82427872e-01  -1.22539564e-01  -1.00844742e+00  -1.18707296e+00
   -3.57170564e-01   1.43672534e+00]
 [ -5.16575904e-01  -5.96124943e-01  -1.49768243e+00  -5.71985026e-01
    1.18285232e+00  -3.85196463e-01  -3.28042340e-01  -1.87868432e+00
    2.87559046e-01  -9.68149789e-02]
 [  2.17861091e+00   7.19657977e-01   3.44953952e-01  -8.22928846e-01
    1.77570729e+00   7.00211315e-01  -2.07695790e+00  -1.41482472e+00
    1.10388314e+00   1.32500136e+00]
 [ -1.02570053e+00   1.57886815e+00   1.29796026e+00   4.16239032e-01
    1.25294762e+00   3.62039055e-02   4.86538339e-01   1.92117054e+00
    2.94275635e-01  -2.12145535e+00]
 [  1.01821253e+00  -3.62229904e-01   1.24953113e+00  -7.87755521e-01
   -1.92401764e-01  -1.42230643e+00   1.00896705e-01   2.05603117e-03
    1.80184012e+00  -1.35195352e+00]]

amueller · 2015-03-19T02:41:07Z

Can you just give the random state seed? Also, do you have any idea what the problem could be?

vinayak-mehta · 2015-03-19T13:23:08Z

I think RandomState solved it.

landscape-bot · 2015-03-19T13:34:13Z

Repository health increased by 0.01% when pulling e266cd2 on vortex-ape:move_test into d69f2d6 on scikit-learn:master.

No new problems were introduced.
1 problem was fixed (including 0 errors and 1 code smell).

amueller · 2015-03-19T15:40:14Z

Well but it should work for all seeds, not only for one ;)

vinayak-mehta · 2015-03-20T04:28:24Z

Is there any problem in hess(x)? Is it the right way to do it? I will search more about it, though it would be awesome if someone who has in-depth knowledge about newton_cg could help :)

amueller · 2015-03-20T18:00:14Z

sklearn/utils/tests/test_optimize.py

why is this different from hess? It seems you use two different interfaces. if you use this here, maybe use fhess in fmin_ncg instead fhess_p?

Thought the results really shouldn't depend on that.

amueller · 2015-03-20T18:06:00Z

just set tol in newton_cg to 1e-10 and it works.

vinayak-mehta · 2015-03-20T23:19:17Z

Some cases show convergence warnings for newton_cg but I think that shouldn't be a problem as assert doesn't raise any error.

landscape-bot · 2015-03-20T23:35:28Z

Repository health increased by 0.01% when pulling 16bacfa on vortex-ape:move_test into d69f2d6 on scikit-learn:master.

No new problems were introduced.
1 problem was fixed (including 0 errors and 1 code smell).

amueller · 2015-03-23T14:49:31Z

Ok thanks, lgtm.

GaelVaroquaux · 2015-03-24T06:49:07Z

sklearn/utils/tests/test_optimize.py

Can you please use a local random number generator with a specific seed:

rng = np.random.RandomState(0) A = rng.normal(size=(10, 10))

GaelVaroquaux · 2015-03-24T06:49:30Z

+1 for merge after the use of a local RNG

simplified code Used fhess_p Added RandomState Added tol=1e-10 to newton_cg Added RandomState with seed 0

vinayak-mehta · 2015-03-24T07:00:18Z

@GaelVaroquaux - Used a local RNG and squashed the commits.

GaelVaroquaux · 2015-03-24T07:01:30Z

Good job! Will merge once travis is green.

vinayak-mehta · 2015-03-24T07:03:47Z

Ok :)

landscape-bot · 2015-03-24T07:04:34Z

Repository health increased by 0.00% when pulling 0cf41c8 on vortex-ape:move_test into 6e8ad67 on scikit-learn:master.

1 new problem was found (including 0 errors and 1 code smell).
2 problems were fixed (including 0 errors and 2 code smells).

[MRG + 2] Fixes #4385: Move newton_cg test out of optimize

coveralls · 2015-03-24T07:44:08Z

Coverage increased (+0.02%) to 95.11% when pulling 0cf41c8 on vortex-ape:move_test into 6e8ad67 on scikit-learn:master.

amueller reviewed Mar 16, 2015
View reviewed changes

sklearn/utils/tests/test_optimize.py Outdated

Copy link

Member

amueller Mar 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole should wrapped in a test_newton_cg function.

vinayak-mehta force-pushed the move_test branch from 5ad1365 to aed7460 Compare March 17, 2015 21:58

amueller reviewed Mar 20, 2015
View reviewed changes

amueller changed the title ~~Fixes #4385: Move newton_cg test out of optimize~~ [MRG + 1] Fixes #4385: Move newton_cg test out of optimize Mar 23, 2015

GaelVaroquaux reviewed Mar 24, 2015
View reviewed changes

Moved optimize test to a new file

0cf41c8

simplified code Used fhess_p Added RandomState Added tol=1e-10 to newton_cg Added RandomState with seed 0

vinayak-mehta force-pushed the move_test branch from 16bacfa to 0cf41c8 Compare March 24, 2015 06:59

GaelVaroquaux changed the title ~~[MRG + 1] Fixes #4385: Move newton_cg test out of optimize~~ [MRG + 2] Fixes #4385: Move newton_cg test out of optimize Mar 24, 2015

GaelVaroquaux added a commit that referenced this pull request Mar 24, 2015

Merge pull request #4389 from vortex-ape/move_test

6354e45

[MRG + 2] Fixes #4385: Move newton_cg test out of optimize

GaelVaroquaux merged commit 6354e45 into scikit-learn:master Mar 24, 2015

vinayak-mehta deleted the move_test branch March 24, 2015 07:18

Uh oh!

[MRG + 2] Fixes #4385: Move newton_cg test out of optimize #4389

[MRG + 2] Fixes #4385: Move newton_cg test out of optimize #4389

Uh oh!

Conversation

vinayak-mehta commented Mar 13, 2015

Uh oh!

landscape-bot commented Mar 13, 2015

Uh oh!

amueller Mar 16, 2015

Choose a reason for hiding this comment

Uh oh!

landscape-bot commented Mar 17, 2015

Uh oh!

amueller commented Mar 18, 2015

Uh oh!

vinayak-mehta commented Mar 18, 2015

Uh oh!

amueller commented Mar 18, 2015

Uh oh!

vinayak-mehta commented Mar 19, 2015

Uh oh!

amueller commented Mar 19, 2015

Uh oh!

vinayak-mehta commented Mar 19, 2015

Uh oh!

landscape-bot commented Mar 19, 2015

Uh oh!

amueller commented Mar 19, 2015

Uh oh!

vinayak-mehta commented Mar 20, 2015

Uh oh!

amueller Mar 20, 2015

Choose a reason for hiding this comment

Uh oh!

amueller Mar 20, 2015

Choose a reason for hiding this comment

Uh oh!

amueller commented Mar 20, 2015

Uh oh!

vinayak-mehta commented Mar 20, 2015

Uh oh!

landscape-bot commented Mar 20, 2015

Uh oh!

amueller commented Mar 23, 2015

Uh oh!

GaelVaroquaux Mar 24, 2015

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented Mar 24, 2015

Uh oh!

vinayak-mehta commented Mar 24, 2015

Uh oh!

GaelVaroquaux commented Mar 24, 2015

Uh oh!

vinayak-mehta commented Mar 24, 2015

Uh oh!

landscape-bot commented Mar 24, 2015

Uh oh!

coveralls commented Mar 24, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants