beast2
beast2 copied to clipboard
CompEvol
Use Java 9 modules?
Keeping package loading working for both java 8 and java 9+ turns out to be quite hard work, and the current solution (start a Launcher that kicks of a second process with the appropriate classpath) leads to unexpected problems (e.g., BEAST not starting on some Windows 10 installs).
Java 9 does not support a dynamic class path: "Note that Java SE and the JDK do not provide an API for applications or libraries to dynamically augment the class path at run-time." (Java 9 release notes)
Perhaps using Java 9 module system will allow this.
Related to this, it appears packages can interact with each other to the point of things breaking (e.g., report of SNAPP on server problem and #843). Perhaps it would be good to only load those package required to run BEAST.
I recommend we also develop a key word (e.g. import, lib, ...) to determine which packages should be loaded. This can be added as new tags in the XML, or as flags in the comments in the XML.
For example:
<import>
<package="beast2", path=""/>
<package="SNAPP", path="/home/user/.beast/2.5"/>
...
<import/>
Names are case-insensitive and use CBAN package names.
https://compevol.github.io/CBAN/
Java package (+version) information is already in the required attribute of the beast element, and is automatically generated by BEAUti, so I don't think there is a need for more new elements.
I see. So, this information in the required attribute could be used to determine which packages should be loaded during the runtime.
For example:
required="BEAST v2.5.2:BEASTLabs v1.8.2"
will only load classes in beast and beastLab.
In this design, you do not need change the code much.
I started a new branch (issue834) to experiment with a ClassLoader that allows the same behaviour as we had in v2.4 (i.e., loading jars dynamically) that works with Java 9+ and does not require spawning off processes. Commit aafeda4 just adds the class, but has the problem that since beast.jar is loaded by the Application class loader, while packages are loaded by BEASTClassLoader, that package private fields in beast.jar (visible in the Application class loader) are not visible to classes in packages (though visible by classes loaded by BEASTClassLoader). A solution is to put the BEASTClassLoader in launcher.jar, and make AppLaunchers load beast.jar, then invoke main methods from the AppLaunchers.
This could potentially solve #847.
I believe this is due to the relative clock rates being able to escape to extremely low values (<1e-25) causing numerical issues somewhere (not sure where though). Anyway, putting a lower bound on clock rates, like so (in Tim's example XML):
<parameter id="clockrates.c:data" dimension="4" name="stateNode" lower="1e-5">1.0</parameter>
seems to fix the problem (no spikes in the first 100M samples at least). A relative rate of 1e-5 seems quite extreme, so I don't think this would cause any issues if we set this as default lower bound in the BEAUti template.
No arbitrary defaults please! At least make it 1e-9 if you insist on having it. Then we need some way of highlighting it to the user as well, so they know they have arbitrary default priors.
On 11/06/2019, at 8:31 AM, Remco Bouckaert [email protected] wrote:
I believe this is due to the relative clock rates being able to escape to extremely low values (<1e-25) causing numerical issues somewhere (not sure where though). Anyway, putting a lower bound on clock rates, like so (in Tim's example XML):
1.0 seems to fix the problem (no spikes in the first 100M samples at least). A relative rate of 1e-5 seems quite extreme, so I don't think this would cause any issues if we set this as default lower bound in the BEAUti template.— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.
The plot thickens: the default prior on rates is Gamma(0.5, 2), which has a 95% HPD of 0.000982-5.02, so with this prior the problem should occur infrequently enough that it probably does not show up in a trace log.
Tim's example XML has a Gamma(0.05,10) prior which has 95% HPD 5.32e-32 - 5.67 and a mean of 0.5 instead of 1, so the user had some reason to change this prior -- but the lower range of this prior seems highly unrealistic, given these are relative rates. Anyway, with this prior, this XML lets the whole range being explored, and changing the lower bound to 1e-9 gives 95% HPD interval [1.0003E-9, 3.9609] | [1.0009E-9, 4.6786] | [1.0004E-9, 4.0149] | [1.0003E-9, 4.708] for the 4 clock rates, that is, they hug the lower bound (and this is sampling with a tiny little bit of data, not sampling from the prior).
It seems sensible to protect users against numerical issues by setting a lower bound of at least 1e-9 instead of the current bound of 0.
I guess one partition must be all constant?
On 11/06/2019, at 9:11 AM, Remco Bouckaert [email protected] wrote:
The plot thickens: the default prior on rates is Gamma(0.5, 2), which has a 95% HPD of 0.000982-5.02, so with this prior the problem should occur infrequently enough that it probably does not show up in a trace log.
Tim's example XML has a Gamma(0.05,10) prior which has 95% HPD 5.32e-32 - 5.67 and a mean of 0.5 instead of 1, so the user had some reason to change this prior -- but the lower range of this prior seems highly unrealistic, given these are relative rates. Anyway, with this prior, this XML lets the whole range being explored, and changing the lower bound to 1e-9 gives 95% HPD interval [1.0003E-9, 3.9609] | [1.0009E-9, 4.6786] | [1.0004E-9, 4.0149] | [1.0003E-9, 4.708] for the 4 clock rates, that is, they hug the lower bound (and this is sampling with a tiny little bit of data, not sampling from the prior).
It seems sensible to protect users against numerical issues by setting a lower bound of at least 1e-9 instead of the current bound of 0.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.
Not all constants, but not informative either:
<sequence id="seq1" taxon="taxon1" totalcount="4" value="GTCGGTCAGTCA"/>
<sequence id="seq2" taxon="taxon2" totalcount="4" value="TCAGTTAGTCAG"/>
<sequence id="seq3" taxon="taxon3" totalcount="4" value="CAGTCAGTCAGT"/>
Anyway, these last few comments were supposed to be for issue #785, not this current one.
As you can see from the commits above, I have been working on a proposal for v2.7.0 in a fork (so not to interfere with v2.6.x), which aims to generally modernise the code.
Main changes
- refactored to clearly separate package management, base functionality (MCMC, evolution, math) and applications. This means applications (like BEAUti) can be updated without having to go through a full BEAST release.
version.xmlnow requires declaration of all classes you want to make available. This gives the benefits of modules without its drawbacks: packages can use their own version of libraries, or other BEAST packages without having to create a dependency on them, so updating these libraries/packages does not break the package. Examples here and here.- add tests to support the above and prevent introduction of cyclical dependencies.
- migration script to assist making v2.6 packages compatible with v2.7 (more detail here).
- runs under java 8 as well as 17 and uses newer libraries, in particular junit5, antlr and commons math.
- extend set of operators with Bactrian, Adaptable and BICEPS operators for more efficient MCMC inference.
Switch to JavaFX?
One issue is that GUI tests based on fest no longer work under java versions >8. To remedy this, I ported applications to JavaFX in the BeastFX repository, where tests are based on testfx. The look and feel is a bit different, and there is some work to go, but as proof of concept I think it succeeds. One benefit is that it is easy to change themes, and get BEAUti in dark mode (see below). These GUI tests are essential to ensure consistent behaviour of BEAUti, and without them I would not recommend moving away from Java 8. The BeastFX repo is set up so that it can be a complete replacement of the BEAST.app package.
Switch code?
Since there are commits trickling in that should be included in the v2.7, I propose we move the current master branch to a v2.6 branch, and unless there are major objections replace it with the fork. Let me know what you think and whether you have major request for changes for v2.7.
