search

BiologicalRecordsCentre / BRCindicators

An R package for creating indicators from trends data
4 stars 11 forks source link

Altering hyperpriors in bma to avoid numerical problems #75

Open drnickisaac opened 3 years ago

drnickisaac commented 3 years ago

We found a problem whereby 0 was included in the prior range of sd parameters: when converted into precisions, this permits Inf to be included in the distribution of precision parameters, thus causing fail in JAGS. Fix is to add a small number to the hyperpriors. A few other documentation issues fixed

drnickisaac commented 3 years ago

This is failing five separate tests of bma(), where it's expecting the results to equal certain specific values. I thought it was due to the hyperprior changes, but I reverted locally and the same error trips. So I'm wondering if this is related to a change in the RNG, as happened in sparta. https://github.com/BiologicalRecordsCentre/sparta/issues/219 What do you think @AugustT ?

AugustT commented 3 years ago

error relate to checks of BMA, looks like output values don't quite match the expected.

AugustT commented 3 years ago

@drnickisaac I think you might be right. Smells like the same problem we had with sparta. That seemed to be an R version or OS issue with random number generation. This is an on-going problem but I can't think of a better test. Hopefully we can fix this here and they wont mess with the random number generators in the future

AugustT commented 3 years ago

While you are at it did you see the warnings about seed?

��� Warning (testbma.R:17:3): simple run ────────────────────────────────────────
The 'seed' argument will be deprecated in the next version. You can set it yourself with set.seed() instead.
Backtrace:
 1. BRCindicators::bma(...) testbma.R:17:2
 2. jagsUI::jags(...)
 3. jagsUI:::process.input(...)
 4. jagsUI:::gen.inits(inits, n.chains, seed, parallel)

We should change to use set.seed() rather than the seed argument to JAGS. 2 birds 1stone.

drnickisaac commented 3 years ago

@AugustT I think it is related to this discussion about Mac vs Windows errors. and this one too.

It looks like you fixed it in a PR to my branch on 5th Feb. It looks like the fix was as simple as adding set.seed(125) before every example, rather than just the first one.

drnickisaac commented 3 years ago

I've looked at testbma: all but one of the tests already had the seed set explicitly, so that can't explain the multiple failures seen on Travis. Any ideas @AugustT ?

AugustT commented 3 years ago

I think in the sparta case I was able to replicate the error on my PC and then use the new numbers to update my test. I dont think I have done that for the BRCindicators case

AugustT commented 3 years ago

I cant get the random number stuff fixed. Sparta uses R2jags, BRCindicators uses JAGSui, that is the only thing I can see that explains the difference. For now I think we will have to remove this test.

AugustT commented 3 years ago

I get a few warnings At least one Rhat value could not be calculated. I assume this is because the test dataset is so small?

drnickisaac commented 3 years ago

No: that's because the first year estimate is a fixed number, so no variatuon among chains.

On Mon, 29 Mar 2021, 10:32 Dr Tom August, @.***> wrote:

I get a few warnings At least one Rhat value could not be calculated. I assume this is because the test dataset is so small?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BiologicalRecordsCentre/BRCindicators/pull/75#issuecomment-809229420, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6PTKAITIMU2PIKE6LY3Q3TGBJNFANCNFSM4Y3LECJA .

AugustT commented 3 years ago

Oh right, is that a warning we should handle then?

drnickisaac commented 3 years ago

Ideally, yes. It would be great if that warning were suppressed. It is entirely to be expected, but potentially alarming for a naive user. Nick

On Mon, 29 Mar 2021 at 21:38, Dr Tom August @.***> wrote:

Oh right, is that a warning we should handle then?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BiologicalRecordsCentre/BRCindicators/pull/75#issuecomment-809697356, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6PTKA23HXEAVO5ZM77LTDTGDXPHANCNFSM4Y3LECJA .

-- http://drnickisaac.weebly.com/ http://www.ceh.ac.uk/staff/nick-isaac http://www.ceh.ac.uk/staff/nick-isaac @drnickisaac

http://www.britishecologicalsociety.org/getting_involved/special_interest_groups/Macroecology.php

AugustT commented 3 years ago

It seems like travis tests are no longer working @robboyd was going to look into alternatives

DavidRoy commented 3 years ago

@JimBacon is investigating Travis replacement options for Indicia. You can probably stick with Travis as this work is non-commercial? https://github.com/Indicia-Team/warehouse/issues/375