-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Java 9 modules? #834
Comments
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). |
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:
Names are case-insensitive and use CBAN package names. |
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 For example: 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 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):
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 ***@***.***> 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):
<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.
—
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 ***@***.***> 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:
Anyway, these last few comments were supposed to be for issue #785, not this current one. |
This reverts commit 871a749.
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
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. |
It contains two packages: beast.pkgmgmt and beast.base. The beast.app package is move to [BEAST.app](https://github.com/CompEvol/BEAST.app), which is Swing based and [BeastFX](https://github.com/CompEvol/BeastFX), which is JavaFX based. The former does not allow GUI unit testing with java version over 8, while the latter does, and has a number of new features built int like `help me choose` buttons, theme chooser and method section generation. Currently, the code is still Java 8 compatible, but if we go with BeastFX we can move to Java 17. [scripts/migrate.md](https://github.com/CompEvol/beast2/tree/master/scripts/migrate.md) contains some information on how to migrate v2.6 packages to v2.7. Most notably, classes moved to get a more logical structure so imports need to be adjusted (a [script](https://github.com/CompEvol/beast2/tree/master/scripts/migrate.pl) is available to assist), and all BEAST objects need to be declared in the [`version.xml`](https://github.com/CompEvol/beast2/tree/master/version.xml) file.
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.
The text was updated successfully, but these errors were encountered: