BiologicalRecordsCentre / BRCindicators

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

More flexible options for scaling the indicator line #65

Closed drnickisaac closed 4 years ago

AugustT commented 4 years ago

@drnickisaac Can you please add tests that check this new functionality?

drnickisaac commented 4 years ago

Not anytime soon - this coding has been a displacement activity from grant-writing ....

AugustT commented 4 years ago

@drnickisaac Are you likely to have time for this now? Else I might write a bunch of tests now. I might see if I can fork your version, write the test there and them pull request into yours, that would mean I wouldn't pull tests into the master before we know they are appropriate.

AugustT commented 4 years ago

In a move I did not know was possible I have pulled your fork into my fork, so now I have your changes in my fork and I can work on the tests there. Then once I have the tests written I can put in a pull request back to your fork.

I think that makes sense?!

AugustT commented 4 years ago

I'm having problems with model smooth_det

set.seed(123)

# Create data for testing
data <- data.frame(species = rep(letters, each = 50),
                   year = rep(1:50, length(letters)),
                   index = rnorm(n = 50 * length(letters), mean = 0, sd = 1),
                   se = runif(n = 50 * length(letters), min = 0.01, max = .1))

  bma_indicator_det <- bma(data,
                       model = "smooth_det",
                       m.scale = "logit",
                       n.iter = 100)

gives me

Error in jags.model(file = model.file, data = data, inits = inits, n.chains = n.chains,  : 
  RUNTIME ERROR:
Compilation error on line 27.
Unknown variable tau.sg
Either supply values for this variable with the data
or define it  on the left hand side of a relation.

@drnickisaac any ideas?

AugustT commented 4 years ago

On line 34 the specification of what is returned is out of date

AugustT commented 4 years ago

When I set incl.2deriv = TRUE I cannot spot any differences in the output... what is supposed to happen?

drnickisaac commented 4 years ago

Ok, I fixed the tau.sg problem in my local version. Re line 34: I don't know which file you are referring to. Perhaps raise as a separate issue on my fork? incl.2.deriv = TRUE does a bunch of extra work but we're not currently monitoring the output. It can be fixed by monitoring an additional parameter t2dash. Again, this needs it's own issue

drnickisaac commented 4 years ago

Does your version run the other tests satisfactorily? Mine throws a wobbly in the "different parameters" section.

drnickisaac commented 4 years ago

I've added a line in bayesian_meta_analysis() to monitor the parameter t2dash if incl.2deriv = TRUE

drnickisaac commented 4 years ago

I've now tested the second derivative code and added lines to make it work without errors. I've also deprecated the smooth_det option, since it contains an error (I was intending to remove it eventually). The code is still accessible from get_bmBUGScode() but not bma().

drnickisaac commented 4 years ago

These changes are now contained in this pull request, along with Tom's tests.

AugustT commented 4 years ago

@drnickisaac This was the return line I was talking about (bma function @return section): https://github.com/BiologicalRecordsCentre/BRCindicators/pull/65/files#diff-d38592f2bee9e42141dd3fbf19ec2504L33

it says it returns, "a dataframe with 4 columns: Year, Index, lower2.5, upper97.5. The last two columns are the credible intervals", but when I do str on the output I get:

'data.frame':   50 obs. of  7 variables:
 $ Year          : num  1 2 3 4 5 6 7 8 9 10 ...
 $ Index.Mprime  : num  100 93.3 90.6 87.5 85.7 ...
 $ lowerCI.Mprime: num  100 86.5 84.1 78.4 75.4 ...
 $ upperCI.Mprime: num  100 101.7 100.4 97.4 99 ...
 $ Index.M       : num  100 95.1 91.3 88.5 86.4 ...
 $ lowerCI.M     : num  100 89.9 82.5 77.1 73.3 ...
 $ upperCI.M     : num  100 100 101 101 102 ...

These aren't the same. As a user I also don't know what Mprime and M are. I think a small addition to the @returns line should be added so that the user can interpret what is returned.

AugustT commented 4 years ago

Does your version run the other tests satisfactorily? Mine throws a wobbly in the "different parameters" section.

Yes the rest worked fine for me

drnickisaac commented 4 years ago

Ok - I see this. I have now fixed this locally and will Push the changes ASAP.

drnickisaac commented 4 years ago

NB I've also changed the DESCRIPTION file, iterating the version to 1.3.6

drnickisaac commented 4 years ago

@AugustT I think this is ready to merge now

AugustT commented 4 years ago

@drnickisaac It's gone in! Do you want me to create a release of this version or do you think there is any chance you will make changes before your manuscript is published.