Skip to content

Conversation

ctm22396
Copy link
Contributor

In which the 'scaling' variable is initialized as an array of ones instead of a dict that matches variables with arrays of ones with the shape of the variable. This resulted in the 'scaling' variable have the shape of entire model. So, if a user set the step method manually for a variable, the shape of the '

This commit preserves the intentions of the 6f58dbf commit by initializing 'scaling' to a dict of arrays of ones with type floatX. As a result, the function "guess_scaling" will be able to create a scaling array with the correct shape.

…itialized as an array instead of a dict. This commit maintains the intentions of the 6f58dbf commit by initializing 'scaling' to a dict of arrays of identity with type floatX.
@junpenglao
Copy link
Member

this is related to #2413 right?

@ctm22396
Copy link
Contributor Author

correct

@aseyboldt
Copy link
Member

@ctm22396 Thanks for the pull request. You are right that the current behaviour is wrong. But the original intension of 6f58dbf was to avoid using guess_scaling if nothing is specified, as this requires computing the hessian, which is expensive, not implemented in some cases, and useless in most cases.
Instead, it should default to an array of ones of dtype theano.config.floatX with the total number of parameters in all its variables as length.

@ctm22396
Copy link
Contributor Author

ctm22396 commented Jul 15, 2017

Oh I see. So perhaps the code could take the subset of the test point dictionary containing the requisite variables and build the array of ones from that?

if scaling is None and potential is None:
    varnames = [var.name for var in vars]
    subset = {k: v for k, v in model.test_point.items() if k in varnames}
    bij = DictToArrayBijection(ArrayOrdering(vars), subset)
    scaling = floatX(np.ones(bij.map(subset).size))

@junpenglao
Copy link
Member

@ctm22396 I think changing it to scaling = floatX(np.ones(len(vars))) should be sufficient.

@ctm22396
Copy link
Contributor Author

ctm22396 commented Jul 15, 2017

@junpenglao

The length of 'vars' will not be the same as the size of the array form of the dictionary if the model has random variables that have shapes > 1.

Perhaps cutting out the dict to array function and doing this is better:

if scaling is None and potential is None:
    varnames = [var.name for var in vars]
    size = sum(v.size for k, v in model.test_point.items() if k in varnames)
    scaling = floatX(np.ones(size))

I was curious, and the native python sum function is faster on python objects (i.e. this iterator) according to https://stackoverflow.com/questions/10922231/pythons-sum-vs-numpys-numpy-sum . The decrease in performance in np.sum is due to the overhead of converting the iterable into a numpy array before summing.

@ctm22396 ctm22396 changed the title Fixed error introduced by 6f58dbf Fixed error introduced by 6f58dbf (https://github.com/pymc-devs/pymc3/issues/2413) Jul 15, 2017
@ctm22396 ctm22396 changed the title Fixed error introduced by 6f58dbf (https://github.com/pymc-devs/pymc3/issues/2413) Fixed error introduced by 6f58dbf Jul 15, 2017
@ctm22396
Copy link
Contributor Author

I believe I correctly reran the tests that it had previously failed on (test_leapfrog_reversible and test_leap_reversible_single). There were no errors this time.

Also, this syntax is compatible with python 2.7

@junpenglao
Copy link
Member

@ctm22396 Yep you are right, please go ahead and push the new change.

ctm22396 added 2 commits July 16, 2017 09:00
This commit is actually preserves the intention of the previous commit as it avoids the guess_scaling function. It still maintains that the shape of scaling must be the size of the specified variables instead of the whole model.
@ctm22396
Copy link
Contributor Author

Thanks for the help!

@aseyboldt
Copy link
Member

@ctm22396 Thanks for the fix.

@aseyboldt aseyboldt merged commit 971db07 into pymc-devs:master Jul 16, 2017
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