search

greta-dev / greta.dynamics

a greta extension for modelling dynamical systems
https://greta-dev.github.io/greta.dynamics/
Other
6 stars 2 forks source link

r_iterate_matrix function in tests returns incorrect stable state if loop reaches niter #15

Open jdyen opened 5 years ago

jdyen commented 5 years ago

I think there is a mismatch in the stable states returned by the r_iterate_matrix and iterate_matrix functions if the loop runs to the final iteration. Not sure if current tests are ending the loop before niter, hence not erroring.

The (possible) issue arises when the while loop never breaks due to diff > tol, in which case the final iteration is (i < niter => i = niter - 1). This iteration increments i by 1, then stores the output to states[[i+1]]. So the final element of states is at niter + 1. https://github.com/greta-dev/greta.dynamics/blob/e89309d2e851373adc38e7d3eb77e9175ef9dc5c/tests/testthat/helpers.R#L23-L25

But then the stable state is returned as states[[i]], which is states[[niter]] in this case: https://github.com/greta-dev/greta.dynamics/blob/e89309d2e851373adc38e7d3eb77e9175ef9dc5c/tests/testthat/helpers.R#L32

Is this intentional? If not, I can prepare a PR. The same issue arises on the iterate_dynamic_matrix branch.

jdyen commented 5 years ago

In fact, this should cause an issue in all cases, shouldn't it? I can't decipher the tf_extract_stable_distribution function so am not sure whether this pulls out the last or second-to-last iteration.

goldingn commented 5 years ago

One difference is that the stable state will be normalised (to sum to 1), but the last nonzero element of all states isn't. Did you account for that?

Even if it's the penultimate state, the convergence criterion should mean it's the same as the ultimate one (when normalised, and to within the tolerance).

goldingn commented 5 years ago

Oh, I misread this, sorry. Not sure, but will take a look when my brain is less addled!