clarify documentation for @return in chain_sim / change behavior for tree + serial combinations

[pearsonca]

The documentation for return value in chain_sim suggests that all combinations of serial and tree are allowed, but function complains if tree=FALSE with a serial function defined.

This suggests either a documentation or behavior bug.

Relative to behavior option, I have a case where I don't need id, generation, ancestor information, but do need time and am memory constrained, so disregarding those would be useful.

[sbfnk]

Thanks for the note.

The documentation of serial states (though perhaps not clearly enough) that setting it implies tree=TRUE:
https://github.com/sbfnk/bpmodels/blob/466896dd57c5dd51ef9cbe7c0cff4cdd41bbfda4/man/chain_sim.Rd#L26

As for the point about memory, some sort of ancestor tracking is required with serial=TRUE, I think, to calculate the infection times of new offspring as they depend on the infection times of the ancestor. There might be a way to save memory (which would be a great thing) by only storing the last generation, but this would require a bit of a rewrite of how this is handled in chain_sim - happy to consider a pull request if you have the capacity to have a go at this.

On a related note, the rbinding of data frames in the same function is probably horrendously inefficient and worth looking at sometime in the future.

[pearsonca]

I hear there is a coding event coming up at LSHTM; seems like a reasonable candidate for a project proposal to take this repo overall to a CRAN package?

…

On Tue, Mar 10, 2020 at 8:03 PM Sebastian Funk ***@***.***> wrote: Thanks for the note. The documentation of serial states (though perhaps not clearly enough) that setting it implies tree=TRUE: https://github.com/sbfnk/bpmodels/blob/466896dd57c5dd51ef9cbe7c0cff4cdd41bbfda4/man/chain_sim.Rd#L26 As for the point about memory, some sort of ancestor tracking is required with serial=TRUE, I think, to calculate the infection times of new offspring as they depend on the infection times of the ancestor. There might be a way to save memory (which would be a great thing) by only storing the last generation, but this would require a bit of a rewrite of how this is handled in chain_sim - happy to consider a pull request if you have the capacity to have a go at this. On a related note, the rbinding of data frames in the same function is probably horrendously inefficient and worth looking at sometime in the future. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN6JR37AQKIE47M3FP57MDRG3INDANCNFSM4LFID4LQ> .

[sbfnk]

There's nothing as such that prevents this from going on CRAN as is - but all improvements would of course be most welcome!