Skip to content
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

Move 'seed' parameter from Tellurium extension to roadrunner core #457

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

evilnose
Copy link
Contributor

This tiny fix addresses roadrunner issue #578.

@evilnose evilnose requested a review from luciansmith June 22, 2020 22:33
@luciansmith
Copy link
Contributor

Looks like the checks failed, probably because there are tests that rely on 'seed' being an argument for the function in question. Looks like it didn't even build. I'm not 100% sure that it would have built before your change, but backing it out for now and checking again seems like the right thing to do.

@evilnose
Copy link
Contributor Author

I think this shouldn't cause any additional tests to fail, since I only modified the docstring and not the code. But in any case, gillespie directly calls RoadRunner.simulate with its args and kwargs but RoadRunner.simulate does not have an argument named "seed", so calling it with "seed" would fail.

We could perhaps modify the code here to extract the "seed" argument and discard it from the kwargs being passed to roadrunner, although we already have setSeed so I'm not sure if it's necessary.

@evilnose
Copy link
Contributor Author

I just saw the updates to #578. If it's decided to transfer this code to roadrunner I can modify this PR to do that instead.

@luciansmith
Copy link
Contributor

I think the seed argument is caught by Tellurium itself (somehow?) or at least is supposed to be caught by Tellurium. I only know this because of comments from Kiri and Kyle, and I don't know any details, unfortunately.

It looks like we need to figure out how to make Travis pass again for first, and then worry about moving the functionality into Roadrunner. Does that seem reasonable?

@evilnose
Copy link
Contributor Author

Ah I see. Then for sure, that sounds like the right thing to do. Thanks for pointing this out to me.

@matthiaskoenig
Copy link
Collaborator

Not sure removing the seed parameter from the signature is the right approach.

Users want to be able to set the seed for gillespie simulations. How else would one reproduce single stochastic trajectories with vanilla roadrunner? Or do I misunderstand stomething here?

@luciansmith luciansmith changed the title remove 'seed' parameter from gillespie signature Move 'seed' parameter from Tellurium extension to roadrunner core Jun 23, 2020
@luciansmith
Copy link
Contributor

@matthiaskoenig : I updated the title of the issue to reflect what we eventually concluded. When the 'seed' parameter was added to the function, it was added in Tellurium by extending the roadrunner function somehow. Instead, we want to add it to the core functionality of roadrunner itself.

@evilnose
Copy link
Contributor Author

@matthiaskoenig Before gillespie() is fixed to accept a "seed" argument and moved to roadrunner core, right now another way to set the seed is to use setSeed() if you have tellurium imported. If you only have roadrunner you could also use rr.getIntegratorByName('gillespie').seed = 123.

@matthiaskoenig
Copy link
Collaborator

Thanks for the quick answer. Just wanted to make sure that it is possible to set a seed on the stochastic simulators.
Best Matthias

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