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

Require at least one Epoch in a Deme #25

Closed
jeromekelleher opened this issue Dec 4, 2020 · 8 comments
Closed

Require at least one Epoch in a Deme #25

jeromekelleher opened this issue Dec 4, 2020 · 8 comments
Labels
demes-python This needs changes in demes-python too.
Milestone

Comments

@jeromekelleher
Copy link
Member

Currently we're implicitly defining a single epoch via some attributes of the Deme class. From a specification/parser writing perspective, it would be considerably simpler to just required the existence of at least one epoch. So, from the examples:

demes:
  constant_size_deme:
    initial_size: 1000
demes:
  constant_size_deme:
    epochs:
    - initial_size: 1000
      end_time: 0

Is the first that much more readable than the second? Are we the models that people will be writing down to contain many demes with just one epoch?

@grahamgower
Copy link
Member

There was some discussion, sorry, I forgot. popsim-consortium/demes-python#133

@grahamgower
Copy link
Member

Are we [expecting] the models that people will be writing down to contain many demes with just one epoch?

That will probably be a frequent occurrence, if these two examples are anything to go by.
https://github.com/popsim-consortium/demes-python/blob/main/examples/gutenkunst_ooa.yml
https://github.com/popsim-consortium/demes-python/blob/main/examples/jacobs_papuans.yml

@jeromekelleher
Copy link
Member Author

Thanks @grahamgower, very helpful.

So, I think the earlier discussion was conflating the demes-python implementation and the definition of the specification. There's nothing stopping an implementation keeping start/end time attributes per deme (and I agree this is a useful thing), but it's not necessary from the specification perspective.

The question is, is the slightly more concise yaml description worth the extra complexity? I'm leaning towards "no".

@grahamgower
Copy link
Member

The question is, is the slightly more concise yaml description worth the extra complexity? I'm leaning towards "no".

100% agreed! Having to write epochs: and indent another level doesn't make the yaml any harder to write, nor does it make mistakes any more likely.

@grahamgower grahamgower added this to the 1.0 milestone Dec 4, 2020
@grahamgower grahamgower added the demes-python This needs changes in demes-python too. label Dec 4, 2020
@jeromekelleher
Copy link
Member Author

@apragsdale, thoughts?

@apragsdale
Copy link
Member

I go back and forth on that issue. To me, the extra lines and indentation are noticeable, but I also think there's something to be said for having the YAML description be more explicit: it forces the user to think in terms of epochs which I think will reduce confusion and implementation errors.

I'm more worried about human-readability and reducing implementation errors than the complexity of the code to parse the reduced representation. But I think we can have both by requiring epochs.

@jeromekelleher
Copy link
Member Author

OK, sounds like unanimity so far - @molpopgen, any dissent?

@molpopgen
Copy link
Contributor

molpopgen commented Dec 4, 2020 via email

@jeromekelleher jeromekelleher changed the title Require at least one Epoch in a Deme? Require at least one Epoch in a Deme Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
demes-python This needs changes in demes-python too.
Projects
None yet
Development

No branches or pull requests

4 participants