Skip to content

Fix nondeterministic preprocessing extension - #145

Open
aryamanDutta wants to merge 1 commit into
GeomScale:developfrom
aryamanDutta:fix-136-preprocess-determinism
Open

Fix nondeterministic preprocessing extension#145
aryamanDutta wants to merge 1 commit into
GeomScale:developfrom
aryamanDutta:fix-136-preprocess-determinism

Conversation

@aryamanDutta

Copy link
Copy Markdown

**Fixes #136.

What does this implement/fix? Describe your changes.

PreProcess.reduce(extend=True) internally uses MCMC sampling to obtain steady states, which are then used to calculate reaction correlations. Due to the use of an internal random number generator, setting np.random.seed() has no effect on the reproducibility of this particular random number generator.

This PR adds the possibility to supply precomputed steady_states to reduce():

  • If steady_states are supplied when calling reduce(), internal MCMC sampling is not performed, and the provided steady_states are used for computing the reaction correlation matrix. In this case, the steady_states argument is validated to be a 2D array with shape (number of reactions in reduced model) x (number of samples).

  • If steady_states are not supplied, the internal MCMC sampling is performed; however, a UserWarning is raised to remind the user that reproducibility of results is not guaranteed.

Tests

Modified tests cover the cases when user-supplied steady_states are used in the calculation of the reaction correlation matrix with extend=True, as well as invalid dimensions of such steady_states. All the tests passed:

Ran 27 tests in 0.628s

OK**

@aryamanDutta

Copy link
Copy Markdown
Author

Hey mentors I’ve opened this PR to address #136. I traced the reproducibility issue to the internal MCMC sampling used by PreProcess.reduce(extend=True). The PR adds support for supplying precomputed steady_states to bypass that sampling while preserving the existing behavior with a warning when they aren't provided.
I’ve also added validation and tests for the new path. Would appreciate your review when you get a chance.

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.

Non-Deterministic results PreProcess.reduce(extend=True) due to internal MCMC sampling

1 participant