-
-
Notifications
You must be signed in to change notification settings - Fork 303
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
Refactor markov #99
Refactor markov #99
Conversation
@@ -400,7 +365,7 @@ initial states. | |||
""" | |||
function simulate!(mc::MarkovChain, X::Matrix{Int}) | |||
n = size(mc.p, 1) | |||
P_dist = [DiscreteRV(vec(mc.p[i, :])) for i in 1:n] | |||
P_dist = [DiscreteRV(vec(full(mc.p[i, :]))) for i in 1:n] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one main thing I don't feel great about in this PR. Notice that I added full
around each row of the stochastic matrix.
DiscreteRV
uses a cumsum
over an AbstractVector
to give us the transition to the next state. In Julia 0.4 there is no SparseVector
type (only sparse matrix), so to get this to be an AbstractVector
we needed to make it dense before passing to DiscreteRV
. The thing that makes it ok is that cumsum
is an operation that will make any sparse array dense, after the first non-zero element. So we probably are more efficient working directly with the dense array than we would have been working with a dense sparse array, but I just wanted to point this out.
Also, I checked the native machine code for this function with a dense mc.p
before and after making this change and the machine code was identical. this means that when mc.p
is already dense the jit compiler completely removes the no-op associated with calling full
and we have 0 performance hit.
Note, travis failures here are only on julia 0.5 (downstream issue with a dependency not loading on unreleased julia). I have updated the travis.yml file in #100 to allow failures on 0.5, but haven't duplicated that commit here. If there are no objections I'm ok to merge this. |
also simulate_values and value_simulation
Added a few more things:
|
failing tests are a result of a dependency not working on the dev version of Julia. In another PR I have specified that these tests are allowed to fail. We can merge this without waiting for tests to pass. |
Last change for now, I promise: |
This doesn't have to belong to this PR, but |
Good point, see #102 |
This implements a few things:
markov
folderAbstractArray
instead of onlyArray
MarkovChain
type.