CompEvol / beast2

Bayesian Evolutionary Analysis by Sampling Trees
www.beast2.org
GNU Lesser General Public License v2.1
238 stars 83 forks source link

Logger has a both an enum type and an int system for specifying mode #114

Closed tgvaughan closed 10 years ago

tgvaughan commented 10 years ago

I vote for getting rid of the int system.

rbouckaert commented 10 years ago

This means the following release will be a major release, since it breaks MultiMCMC in the BEASTLabs package -- so BEAST will not be backward compatible.

tgvaughan commented 10 years ago

Okay, I'll revert it then.

On 13 May 2014 09:06, rbouckaert notifications@github.com wrote:

This means the following release will be a major release, since it breaks MultiMCMC in the BEASTLabs package -- so BEAST will not be backward compatible.

— Reply to this email directly or view it on GitHubhttps://github.com/CompEvol/beast2/issues/114#issuecomment-42888285 .

rbouckaert commented 10 years ago

Please don't -- it is just a matter of administrating this and keep track of things. It's about time to push through some other things as well.

On Mon, 2014-05-12 at 14:24 -0700, Tim Vaughan wrote:

Okay, I'll revert it then.

On 13 May 2014 09:06, rbouckaert notifications@github.com wrote:

This means the following release will be a major release, since it breaks MultiMCMC in the BEASTLabs package -- so BEAST will not be backward compatible.

— Reply to this email directly or view it on GitHubhttps://github.com/CompEvol/beast2/issues/114#issuecomment-42888285 .

— Reply to this email directly or view it on GitHub.

tgvaughan commented 10 years ago

Ah, sorry - misunderstood your message then. It's really all the same to me - this issue+commit was just a random spring/autumn clean :-)

On 13 May 2014 09:27, rbouckaert notifications@github.com wrote:

Please don't -- it is just a matter of administrating this and keep track of things. It's about time to push through some other things as well.

On Mon, 2014-05-12 at 14:24 -0700, Tim Vaughan wrote:

Okay, I'll revert it then.

On 13 May 2014 09:06, rbouckaert notifications@github.com wrote:

This means the following release will be a major release, since it breaks MultiMCMC in the BEASTLabs package -- so BEAST will not be backward compatible.

— Reply to this email directly or view it on GitHub< https://github.com/CompEvol/beast2/issues/114#issuecomment-42888285> .

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/CompEvol/beast2/issues/114#issuecomment-42890631 .

alexeid commented 10 years ago

I fear that BEASTLabs is going to get large enough (and will continue to grow given its ill-defined scope) so that it will be broken by changes in core with regularity.

It would seem that we need to do two things to avoid this:

(1) Migrate important, mature parts of BEASTLabs into core over time. (2) Avoid putting things into BEASTLabs unless it is a feature that could eventually go into the core.

Otherwise BEASTLabs will become a catch-all for useful but experimental code. This could get rather onerous and could lead to a situation where a de facto requirement of development in BEAST2 is to also depend on and build against BEASTLabs. That would defeat the purpose of modularity, as you would effectively have one monolithic requirement (BEAST2+BEASTLabs) that is just split inconveniently into two repositories with their own versions (and consequent combinatorial explosion of incompatibilities).

MultiMCMC seems an obvious candidate for eventual migration into the core as it is a core feature in other Bayesian phylogenetic packages.

Alexei

On 13/05/2014, at 9:27 am, rbouckaert notifications@github.com wrote:

Please don't -- it is just a matter of administrating this and keep track of things. It's about time to push through some other things as well.

On Mon, 2014-05-12 at 14:24 -0700, Tim Vaughan wrote:

Okay, I'll revert it then.

On 13 May 2014 09:06, rbouckaert notifications@github.com wrote:

This means the following release will be a major release, since it breaks MultiMCMC in the BEASTLabs package -- so BEAST will not be backward compatible.

— Reply to this email directly or view it on GitHubhttps://github.com/CompEvol/beast2/issues/114#issuecomment-42888285 .

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub.

tgvaughan commented 10 years ago

I can't see any reason why BEASTLabs should grow beyond its current size. BEAST has a lovely package management system, and these packages don't need to be large. Rather than adding to BEASTLabs, experimental code can be placed in stand-alone packages that are maintained by the people who wrote the code. This avoids placing the burden of maintaining the compatibility of all of this code on a single person and instead places smaller burdens on those with a vested interest in each particular package. (This could actually be seen as the point of supporting user-contributed packages!)

On 13 May 2014 10:55, Alexei Drummond notifications@github.com wrote:

I fear that BEASTLabs is going to get large enough (and will continue to grow given its ill-defined scope) so that it will be broken by changes in core with regularity.

It would seem that we need to do two things to avoid this:

(1) Migrate important, mature parts of BEASTLabs into core over time. (2) Avoid putting things into BEASTLabs unless it is a feature that could eventually go into the core.

Otherwise BEASTLabs will become a catch-all for useful but experimental code. This could get rather onerous and could lead to a situation where a de facto requirement of development in BEAST2 is to also depend on and build against BEASTLabs. That would defeat the purpose of modularity, as you would effectively have one monolithic requirement (BEAST2+BEASTLabs) that is just split inconveniently into two repositories with their own versions (and consequent combinatorial explosion of incompatibilities).

MultiMCMC seems an obvious candidate for eventual migration into the core as it is a core feature in other Bayesian phylogenetic packages.

Alexei

On 13/05/2014, at 9:27 am, rbouckaert notifications@github.com wrote:

Please don't -- it is just a matter of administrating this and keep track of things. It's about time to push through some other things as well.

On Mon, 2014-05-12 at 14:24 -0700, Tim Vaughan wrote:

Okay, I'll revert it then.

On 13 May 2014 09:06, rbouckaert notifications@github.com wrote:

This means the following release will be a major release, since it breaks MultiMCMC in the BEASTLabs package -- so BEAST will not be backward compatible.

— Reply to this email directly or view it on GitHub< https://github.com/CompEvol/beast2/issues/114#issuecomment-42888285> .

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/CompEvol/beast2/issues/114#issuecomment-42898958 .

alexeid commented 10 years ago

Maybe I wasn't clear, because I totally agree with this. I was just reflecting that BEASTLabs seems to be already rather large and growing.

On 13/05/2014, at 11:12 am, Tim Vaughan notifications@github.com wrote:

I can't see any reason why BEASTLabs should grow beyond its current size. BEAST has a lovely package management system, and these packages don't need to be large. Rather than adding to BEASTLabs, experimental code can be placed in stand-alone packages that are maintained by the people who wrote the code. This avoids placing the burden of maintaining the compatibility of all of this code on a single person and instead places smaller burdens on those with a vested interest in each particular package. (This could actually be seen as the point of supporting user-contributed packages!)

On 13 May 2014 10:55, Alexei Drummond notifications@github.com wrote:

I fear that BEASTLabs is going to get large enough (and will continue to grow given its ill-defined scope) so that it will be broken by changes in core with regularity.

It would seem that we need to do two things to avoid this:

(1) Migrate important, mature parts of BEASTLabs into core over time. (2) Avoid putting things into BEASTLabs unless it is a feature that could eventually go into the core.

Otherwise BEASTLabs will become a catch-all for useful but experimental code. This could get rather onerous and could lead to a situation where a de facto requirement of development in BEAST2 is to also depend on and build against BEASTLabs. That would defeat the purpose of modularity, as you would effectively have one monolithic requirement (BEAST2+BEASTLabs) that is just split inconveniently into two repositories with their own versions (and consequent combinatorial explosion of incompatibilities).

MultiMCMC seems an obvious candidate for eventual migration into the core as it is a core feature in other Bayesian phylogenetic packages.

Alexei

On 13/05/2014, at 9:27 am, rbouckaert notifications@github.com wrote:

Please don't -- it is just a matter of administrating this and keep track of things. It's about time to push through some other things as well.

On Mon, 2014-05-12 at 14:24 -0700, Tim Vaughan wrote:

Okay, I'll revert it then.

On 13 May 2014 09:06, rbouckaert notifications@github.com wrote:

This means the following release will be a major release, since it breaks MultiMCMC in the BEASTLabs package -- so BEAST will not be backward compatible.

— Reply to this email directly or view it on GitHub< https://github.com/CompEvol/beast2/issues/114#issuecomment-42888285> .

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/CompEvol/beast2/issues/114#issuecomment-42898958 .

— Reply to this email directly or view it on GitHub.

rbouckaert commented 10 years ago

I am increasingly warming up to the idea of having rather small packages, given the increasing support for this in BEAUti.

Also, this ensures that BEAST-core remains small and therefore easy to learn, while it is already possible -- due to its current functionality -- to extend it with non-trivial methods. Not everyone has 10 years experience with MCMC methods in phylogenetics and coming to grips with both these as well as BEASTObjects/Inputs/CalculationNodes etc. should be made as easy as possible and a smaller core will do this.

Legacy methods (e.g. MC3) can be migrated to BEAST-classic, while newer methods might need their own packages, or get parked in BEASTLabs while maturing and deciding what to do with.

Remco

alexeid commented 10 years ago

I don't think that MC3 is a "legacy" method (unless MCMC is also legacy?).

I think that beast-classic also has the strong potential to become an evil monolith that is de facto required if we just shove stuff thats important into that package.

I think we need to recognise that there is important and commonly used algorithms and modes that are not in core, but are crucial to many users of BEAST.

The nice thing about putting important stuff into the core (obviously organised in a sensible and modular set of Java packages) is that the code in core gets properly scrutinised, unit tested et cetera. Other packages might not be built against by as many people, so the functionality is more prone to going stale. It fine if important algorithms/methods are in their own add-on as well, so long as they have a champion. But putting them in a catch-all package like beast-classic is not a good idea in my opinion.

Alexei

On 13/05/2014, at 11:27 am, rbouckaert notifications@github.com wrote:

I am increasingly warming up to the idea of having rather small packages, given the increasing support for this in BEAUti.

Also, this ensures that BEAST-core remains small and therefore easy to learn, while it is already possible -- due to its current functionality -- to extend it with non-trivial methods. Not everyone has 10 years experience with MCMC methods in phylogenetics and coming to grips with both these as well as BEASTObjects/Inputs/CalculationNodes etc. should be made as easy as possible and a smaller core will do this.

Legacy methods (e.g. MC3) can be migrated to BEAST-classic, while newer methods might need their own packages, or get parked in BEASTLabs while maturing and deciding what to do with.

Remco — Reply to this email directly or view it on GitHub.

rbouckaert commented 10 years ago

On Mon, 2014-05-12 at 16:35 -0700, Alexei Drummond wrote:

I don't think that MC3 is a "legacy" method (unless MCMC is also legacy?).

I think we need to recognise that there is important and commonly used algorithms and modes that are not in core, but are crucial to many users of BEAST.

A good sign of something being important and commonly used is if it is supported by a GUI. Is MC3 supported by BEAUti1?

The nice thing about putting important stuff into the core (obviously organised in a sensible and modular set of Java packages) is that the code in core gets properly scrutinised, unit tested et cetera. Other packages might not be built against by as many people, so the functionality is more prone to going stale. It fine if important algorithms/methods are in their own add-on as well, so long as they have a champion. But putting them in a catch-all package like beast-classic is not a good idea in my opinion.

The down-side is that it suffers from the danger of making the core bloated. I don't think we want to increase the learning curve by adding lots of code to the core. It is already hard enough.

Given the plans there are for MC3, perhaps it should get its own package, so all MC3 implementations are in the same place.

Remco

alexeid commented 10 years ago

We define important by what we make easy to use. Our expert opinion is presumably more important than what happens to be in BEAST1 or any other Bayesian phylogenetic software package.

For example the ability to resume a chain is not in BEAST1, but we think its important, so its in the BEAST2 core.

Also, the learning curve is not automatically increased by adding more algorithms/methods and Java packages.

The core elements of the Java language and libraries are still easy to learn, even though the standard libraries are now vast. There is an internal modularity and structure to the Java standard libraries that provide a path to learning. We also have internal modularity in the BEAST2 core Java packages. A new developer doesn't need to learn all the classes and Java packages simultaneously in order to learn the core. If we have a richer core, but a clear path to learning the basics, then the learning curve is no steeper, it just means that the learning curve lasts longer, because the library is richer, and the developer eventually can do much more.

If the core is well organised and documented and the learning curve doesn't get steeper to do the simple things, then what is wrong with enriching the core slowly over time?

Alexei

On 13/05/2014, at 11:53 am, rbouckaert notifications@github.com wrote:

On Mon, 2014-05-12 at 16:35 -0700, Alexei Drummond wrote:

I don't think that MC3 is a "legacy" method (unless MCMC is also legacy?).

I think we need to recognise that there is important and commonly used algorithms and modes that are not in core, but are crucial to many users of BEAST.

A good sign of something being important and commonly used is if it is supported by a GUI. Is MC3 supported by BEAUti1?

The nice thing about putting important stuff into the core (obviously organised in a sensible and modular set of Java packages) is that the code in core gets properly scrutinised, unit tested et cetera. Other packages might not be built against by as many people, so the functionality is more prone to going stale. It fine if important algorithms/methods are in their own add-on as well, so long as they have a champion. But putting them in a catch-all package like beast-classic is not a good idea in my opinion.

The down-side is that it suffers from the danger of making the core bloated. I don't think we want to increase the learning curve by adding lots of code to the core. It is already hard enough.

Given the plans there are for MC3, perhaps it should get its own package, so all MC3 implementations are in the same place.

Remco — Reply to this email directly or view it on GitHub.

rbouckaert commented 10 years ago

Either you want to be concerned with bloat or not. If putting classes in BEASTLabs makes it bloated, it will have the same effect on BEAST-core. My preference is to keep things as lean as possible, certainly as far as the core is concerned, and organise other functionality in separate packages.

Remco.

alexeid commented 10 years ago

Not everybody checks out and looks at BEASTLabs, but everybody looks at BEAST2 core. BEAST2 has the critical mass of a core development team and it is the primary focus of all BEAST2 releases.

Java already has the concept of separate packages to organise code within BEAST2 core. It will only become bloated if we put stuff in that isn't supported by the development team and we don't give a clear documentation path (including tutorials) of how to learn the basics.

Alexei

On 13/05/2014, at 1:12 pm, rbouckaert notifications@github.com wrote:

Either you want to be concerned with bloat or not. If putting classes in BEASTLabs makes it bloated, it will have the same effect on BEAST-core. My preference is to keep things as lean as possible, certainly as far as the core is concerned, and organise other functionality in separate packages.

Remco.

— Reply to this email directly or view it on GitHub.

rbouckaert commented 10 years ago

Another reason to keep BEAST-core as small as possible is that releasing bug fixes is a nuisance and a relatively slow process. Releasing packages is a very lightweight exercise -- just zip up the package, put it in an accessible place and update the packages.xml repository file.Releasing BEAST-core requires running the release process on three different operating systems, uploading bulky files, fixing loads of links, etc.

Remco

alexeid commented 10 years ago

Agree very strongly with this. Thus core should only have algorithms that are core, i.e. that almost every add-on/model can use. MCMCMC is a perfect example, since it potentially improves the mixing of any analysis, no matter what the models or add-ons employed.

Alexei

Sent from my iPhone

On 14/05/2014, at 7:41 AM, rbouckaert notifications@github.com wrote:

Another reason to keep BEAST-core as small as possible is that releasing bug fixes is a nuisance and a relatively slow process. Releasing packages is a very lightweight exercise -- just zip up the package, put it in an accessible place and update the packages.xml repository file.Releasing BEAST-core requires running the release process on three different operating systems, uploading bulky files, fixing loads of links, etc.

Remco

— Reply to this email directly or view it on GitHub.