search

lmaurits / BEASTling

A linguistics-focussed command line tool for generating BEAST XML files.
BSD 2-Clause "Simplified" License
20 stars 6 forks source link

Refactor models and clocks #248

Closed xrotwang closed 3 years ago

xrotwang commented 5 years ago

With this PR also clocks and models use sections.Section instances for configuration.

I also refactored clocks and models such that they hold a reference (called options) to the sections.Section object. This means that quite a few attributes on clock and model classes could be removed in favor of accessing the corresponding values via cls.options.<attr>.

xrotwang commented 5 years ago

Oh, and the statement count is well below 3,000 now :)

codecov-io commented 5 years ago

Codecov Report

Merging #248 into master will not change coverage. The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #248   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          29     32    +3     
  Lines        3014   2936   -78     
=====================================
+ Misses       3014   2936   -78
Impacted Files Coverage Δ
beastling/models/covarion.py 0% <ø> (ø) :arrow_up:
beastling/models/mk.py 0% <ø> (ø) :arrow_up:
beastling/models/binaryctmc.py 0% <ø> (ø) :arrow_up:
beastling/clocks/relaxed.py 0% <0%> (ø) :arrow_up:
beastling/fileio/datareaders.py 0% <0%> (ø) :arrow_up:
beastling/clocks/random.py 0% <0%> (ø) :arrow_up:
beastling/distributions.py 0% <0%> (ø) :arrow_up:
beastling/util/misc.py 0% <0%> (ø) :arrow_up:
beastling/models/basemodel.py 0% <0%> (ø) :arrow_up:
beastling/models/geo.py 0% <0%> (ø) :arrow_up:
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ba26491...bdc3983. Read the comment docs.

xrotwang commented 5 years ago

This PR has now accreted with another two commits, but these two are fairly local and should really go into master because the make for

xrotwang commented 5 years ago

A couple more commits, of which https://github.com/lmaurits/BEASTling/pull/248/commits/795d84f4127b9cf5a0a0f9a7d125e93da01ecfad is critical, also in revealing the problem with the weird caching in tests (that I implemented at a time when the tests felt too slow).

xrotwang commented 5 years ago

I realize that some of these refactorings clash with the way BEASTling is used - e.g. for the D-PLACE pipeline - where Configuration attributes such as families are assigned to directly (rather than by providing appropriate config). I would guess, though, that this has never been the recommended way, since it prevents BEASTling from embedding a reproducible config in the XML.

For the future, I'd aim at considering all Configuration attributes internal, and possibly providing better support for programmatic use by allowing to pass sections.Section instances when initializing Configuration. This would

Anaphory commented 4 years ago

@lmaurits Do you have time in the near future to look over this?

Anaphory commented 4 years ago

I have created issues for the points I noticed in this PR that I thought could be done differently, or that I noticed because of this PR that were not introduced by it. Shall we merge it, @xrotwang and @lmaurits ?