Adding DMEs to a contig

[roshnipatel]

Proposed fix for #1822 - swaps DFEs for DMEs, and preserves backwards compatibility in most places.

Two minor notes to add per @jeffspence:

• in __str__ and mutation_types we break backwards compatibility by replacing "dfe_id" and "dfe_list" (appearing in the string representation) by "dme_id" and "dme_list". Should we worry about this?
• Do we want to update add_single_site to allow a trait effect?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.82%. Comparing base (c96504f) to head (a618cbe).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1827   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files         143      143           
  Lines        5012     5035   +23     
  Branches      513      515    +2     
=======================================
+ Hits         5003     5026   +23     
  Misses          6        6           
  Partials        3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[roshnipatel]

@petrelharp -- @jeffspence wrote pretty much all of this, and I reviewed it before opening the PR, but maybe you or someone else with more institutional knowledge would like to review as well?

[jeffspence]

To give a little bit of context, the main changes are:

In genomes.py:

• References to distributions of fitness effects (DFEs) in docstrings have been changed to distributions of mutational effects (DMEs)
• Contig now has a dme_list attribute, which should be used in place of dfe_list
• To maintain back-compatibility, the constructor in Contig still allows dfe_list as a kwarg but internally it stores this in dme_list (and raise a stdpopsim.DeprecatedFeatureWarning). Contig.dfe_list can still be accessed (it is now a @property) and set (these actually access / set Contig.dme_list and again raise warnings)
• Several functions have had their names changed, with the original versions raising a stdpopsim.DeprecatedFeatureWarning and then calling the new version:
  • Contig.dfe_breakpoints --> Contig.dme_breakpoints
  • Contig.clear_dfes --> Contig.clear_dmes
  • Contig.add_dfe --> Coting.add_dme
• As @roshnipatel mentioned, Contig.mutation_types previously returned a list of dictionaries where one of the keys was "dfe_id" -- now it's "dme_id". This will technically break back-compatibility. If we're concerned about this, we could have the dictionary have both keys.
• The other place where back-compatibility is broken is in Contig.__str__ where the string "dfe_list" appeared in the string representation of the Contig, which again is now changed to "dme_list".

Beyond these main changes, the rest are essentially tests of these in test_genomes.py (generally just backing sure the "dfe" methods raise deprecation warnings, and that the dfe versions and dme versions return the same things), along with moving over from the now deprecated functions to their dme versions in: cli.py, slim_engine.py, traits.py, test_dfes.py, test_engines.py, and test_slim_engine.py.

[petrelharp]

Yes, I'll have a look. Thanks!!

[petrelharp]

in __str__ and mutation_types we break backwards compatibility by replacing "dfe_id" and "dfe_list" (appearing in the string representation) by "dme_id" and "dme_list". Should we worry about this?

I think in __str__ we don't have to worry about it. I don't have the context for mutation_types though - where's that?

[petrelharp]

Suggested change

	By default, the inital interval list spans the whole chromosome with
	DME where variants have no effects on traits or fitness.
	By default, the initial interval list spans the whole chromosome with
	a DME whose variants have no effects on traits or fitness.

[petrelharp]

What's the thinking here? How will this work? (I don't know quite what alias does.)

[petrelharp]

We like to make things explicit rather than relying on truthiness of things (eg is an empty list True or False? I'd have to check), so how about

Suggested change

	if self._dfe_list and self.dme_list:
	if self._dfe_list is not None and self.dme_list is not None:

[petrelharp]

Could you explain the main changes and additions here? Like, it looks like this is this deprecating Contig.dfe_list, is that the intention? An outline of the intention will make it easier to review, eg separating out the "just changed dfe to dme" from actual functionality changes.

Edit: Okay, by my reading this does:

1. observes that Contig.add_dfe mostly just adds the DFE to dfe_list.
2. so, this changes over Contigs to have a dme_list instead of a dfe_list, since DMEs are generalizations of DFEs anyhow.
3. And, this puts a deprecation warning in all the Contig.xxx_dfe_yyy things.

An alternative to the deprecation warnings would be to basically just have all the dfe_list stuff be an alias for dme_list and not bother with the deprecation warnings. It would be cleaner to deprecate and eventually remove it, but we'll be throwing warnings in lots of people's code that works just fine, and DFE is a term they are familiar with (unlike DME). So I'm tempted to not throw those warnings? Thoughts?

[petrelharp]

I don't think this change is correct (at least the first one)?