tgvaughan / MASTER

A versatile simulation engine for stochastic population dynamics models.
http://tgvaughan.github.io/MASTER
GNU General Public License v3.0
22 stars 8 forks source link

automatically set origin value for simulated BeastTreeFromMaster trees #41

Closed denisekuehnert closed 9 years ago

denisekuehnert commented 9 years ago

Hey Tim,

Would you mind pulling this? A few of us need it to run simulations with birth-death models.

Cheers

Denise

tgvaughan commented 9 years ago

Hi Denise, not a problem - but would you mind explaining what this does? The new input you've added doesn't seem to be used anywhere?

denisekuehnert commented 9 years ago

Hey Tim,

The "origin" is a parameter of our birth-death models, parametrizing the time between the first infected individual and the last sample. In some simulations we want to fix the true tree and infer epi-params from it. If we don't care about the origin, we can do the simulation and analysis in a single XML file. Now with this change we can also get the true origin value from MASTER, together with the tree.

Actually in some of the newer coalescent models the origin is also parametrized, so it's not only useful for birth-death models.

Cheers

Denise

2015-07-11 13:47 GMT+02:00 Tim Vaughan notifications@github.com:

Hi Denise, not a problem - but would you mind explaining what this does? The new input you've added doesn't seem to be used anywhere?

— Reply to this email directly or view it on GitHub https://github.com/CompEvol/MASTER/pull/41#issuecomment-120607002.

tgvaughan commented 9 years ago

Hi Denise, I just don't see where it's used in the code though. You have an Input named originInput, but its value is never retrieved. For example, do you have some other class that extends BeastTreeFromMaster where you're using originInput? If so, wouldn't it make sense to move originInput to that child class?

An alternative to this is to use a non-zero root branch length to keep track of the origin. This is what I do in the NewickOutput code.

tgvaughan commented 9 years ago

Sorry for being so picky - it just looks weird to add an input and have it not actually used in the class that provides it. Maybe the best solution would be to add an origin field to the Tree class in BEAST?

denisekuehnert commented 9 years ago

Julia is using it in her XML files. We don't have another extending class.

I agree that the cleanest solution would be to add it to the tree, but that would make it a major change for all of the birth-death tree priors in BEAST2. I don't think that's worth it at this point.

Anyway, if you'd rather not pull it, that's fine. Just let me know.

2015-07-13 23:53 GMT+02:00 Tim Vaughan notifications@github.com:

Sorry for being so picky - it just looks weird to add an input and have it not actually used in the class that provides it. Maybe the best solution would be to add an origin field to the Tree class in BEAST?

— Reply to this email directly or view it on GitHub https://github.com/CompEvol/MASTER/pull/41#issuecomment-121073334.

tgvaughan commented 9 years ago

Hey Denise, no worries I've merged it. We should try to find a better solution sometime though!

tgvaughan commented 9 years ago

I guess what puzzles me is that originInput is being used to provide a number (the origin) to BeastTreeFromMaster, but that number is never used by BeastTreeFromMaster. Presumably this number is used by some other class which exploits the fact that all Inputs must be public.

Given the fact that BeastTreeFromMaster is neither generating the origin value itself nor using the origin value that originInput provides, shouldn't the input be added to whatever class is actually generating/using the value?

denisekuehnert commented 9 years ago

The value is generated by the class InheritanceTrajectory. But the InheritanceTrajectory object is created within BeastTreeFromMaster and not accessible from the XML file.

2015-07-15 4:29 GMT+02:00 Tim Vaughan notifications@github.com:

I guess what puzzles me is that originInput is being used to provide a number (the origin) to BeastTreeFromMaster, but that number is never used by BeastTreeFromMaster. Presumably this number is used by some other class which exploits the fact that all Inputs must be public.

Given the fact that BeastTreeFromMaster is neither generating the origin value itself nor using the origin value that originInput provides, shouldn't the input be added to whatever class is actually generating/using the value?

— Reply to this email directly or view it on GitHub https://github.com/CompEvol/MASTER/pull/41#issuecomment-121461897.

tgvaughan commented 9 years ago

Ah, I see - sorry, I missed the statenode initialization bit!! I understand now, sorry for being so slow! I suppose the reason it took me so long is that I hardly ever use the StateNodeInitializer idiom (as you can probably tell by my code).