lmaurits / BEASTling

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

config.prior vs. config.sample_from_prior #220

Open Anaphory opened 5 years ago

Anaphory commented 5 years ago

I don't think the implementation of the command line argument versus the config file option for sampling from prior is clean.

I think it should be like this:

So, this line https://github.com/lmaurits/BEASTling/blob/b422d1644e118d87e78c50e7d50194bb21f69edb/beastling/configuration.py#L98 is correct.

https://github.com/lmaurits/BEASTling/blob/b422d1644e118d87e78c50e7d50194bb21f69edb/beastling/configuration.py#L170 only serves for generic value reading from the config file and if we ever do that well for documenting that config file parameter.

https://github.com/lmaurits/BEASTling/blob/b422d1644e118d87e78c50e7d50194bb21f69edb/beastling/configuration.py#L194 suggests that it is useful to keep prior around as config attribute, and I think this is wrong: Only one of sample_from_prior and prior should be kept alive, in particular to avoid confusion further down:

https://github.com/lmaurits/BEASTling/blob/b422d1644e118d87e78c50e7d50194bb21f69edb/beastling/configuration.py#L293 overloads self.sample_from_prior with self.prior, but then the subsequent lines use self.prior, and one even duplicates the functionality from L98 (see above), but in a slightly more clever (too clever, in particular once we implement proper config-interpolation) way.

https://github.com/lmaurits/BEASTling/blob/b422d1644e118d87e78c50e7d50194bb21f69edb/beastling/beastxml.py#L144 is where this is used in building the XML, suggesting that .sample_from_prior is the canonical attribute.

To Do

Remove all traces of the config.prior attribute, replacing it by config.sample_from_prior or the function argument prior as appropriate.