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

Interpretation of multiple "identical" pulses #155

Open
molpopgen opened this issue Jun 28, 2022 · 6 comments
Open

Interpretation of multiple "identical" pulses #155

molpopgen opened this issue Jun 28, 2022 · 6 comments

Comments

@molpopgen
Copy link
Contributor

I came across this example today when testing something:

time_units: generations
demes:
 - name: A
   epochs:
    - start_size: 50
 - name: B
   epochs:
    - start_size: 50
pulses:
 - sources: [A]
   dest: B
   proportions: [0.9]
   time: 25
 - sources: [A]
   dest: B
   proportions: [0.9]
   time: 25

Currently, demes-python reads the model as valid, emitting a warning that the "outcome depends on the order". fwdpy11 rejects the model because the total rates sum to > 1.0 of deme B's ancestry.

How should this model be interpreted?

Possibly related: #91 and #114

cc @apragsdale b/c it is relevant to the fwdpy11 code.

@apragsdale
Copy link
Member

For a discrete simulator like fwdpy11, I think it is the right thing to reject this model. If a user has tried to simulate with it, they either have messed something up when specifying the model, or they should be thinking about writing clearer models.

In continuous time simulations, they would be applied sequentially, so if you want fwdpy11 to allow this model, you'd want to "collapse" those pulse events, in which case you get A->B with proportion 0.99. But I do think the best thing for discrete models to do is error when they see a pathological model like this.

@grahamgower
Copy link
Member

The spec says these should be applied in the order that they appear in the input. And for this reason, the model should not be interpreted to mean that the rates into B sum to greater than 1.0. So I think that check in fwdpy11 should be removed.

I agree that this particular example is unlikely to be constructed deliberately, and so it would be nice if it were rejected. But maybe the check should instead be for multiple pulses into the same dest deme at the same time. Technically that would be valid, but since it can be rewritten with a single pulse I think it would be ok to reject it.

@molpopgen
Copy link
Contributor Author

@grahamgower -- I find the spec hard to interpret here. It says to apply sequentially and that ancestry proportions are just that, ancestry proportions. It doesn't clearly state that subsequent proportions should be applied to the remaining numbers of individuals. I think it would be easy to interpret multiple pulses as additive.

@grahamgower
Copy link
Member

We discussed this at length previously, across multiple issues. At least #99 and #100. But probably in demes-python issues/PRs too.

The spec currently has:
https://popsim-consortium.github.io/demes-spec-docs/main/specification.html#pulse

Pulse migration events specify the instantaneous replacement of a given fraction of individuals in a destination population by individuals with parents from a source population. The fraction must be between zero and one, and if more than one pulse occurs at the same time, those replacement events are applied sequentially in the order that they are specified in the model. The list of pulses must be sorted in time-descending order.

I'd be happy to review any patches clarifying this!

@grahamgower
Copy link
Member

Oh, and to clarify why we want the current behaviour: it maximises consistency between discrete-time and continuous-time simulators. Imposing additional constraints in any one simulator (like, disallowing multiple pulses at the same time) is perfectly fine, but otherwise it would be really nice for models to be interpreted identically in both cases.

@molpopgen
Copy link
Contributor Author

We discussed this at length previously, across multiple issues. At least #99 and #100. But probably in demes-python issues/PRs too.

The spec currently has: https://popsim-consortium.github.io/demes-spec-docs/main/specification.html#pulse

Pulse migration events specify the instantaneous replacement of a given fraction of individuals in a destination population by individuals with parents from a source population. The fraction must be between zero and one, and if more than one pulse occurs at the same time, those replacement events are applied sequentially in the order that they are specified in the model. The list of pulses must be sorted in time-descending order.

I'd be happy to review any patches clarifying this!

It seems that the discussions never made it into the spec. My point is this: the statement "applied sequentially" implicitly assumes something about how that would be done. In other words, there's something we believe to be a natural implementation that we are not being explicit about. The how is in this comment, but omitted from the spec.

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

No branches or pull requests

3 participants