nimble-dev / nimble

The base NIMBLE package for R
http://R-nimble.org
BSD 3-Clause "New" or "Revised" License
156 stars 23 forks source link

enable AD for CAR models #1390

Closed paciorek closed 8 months ago

paciorek commented 8 months ago

This sets up templated versions of dcar_normal and dcar_proper for AD and enables buildDerivs for various auxiliary nimbleFunctions functions in CAR.R. In the latter case, this requires various uses of ADbreak, ignore and setting up index variables as integers.

Testing now includes tests that AD-enabled CAR models will build and basic (non-HMC) MCMC will run. Substantive HMC testing could be done in nimbleHMC.

@perrydv a glance over the AD-enabled code would be helpful.

@danielturek just an FYI; I'm sure you'll be happy to see this.

paciorek commented 8 months ago

I ran the disease mapping example from our examples with HMC and results compared favorably (in terms of mixing) and were numerically similar to our default MCMC.

Not sure if we want an actual nimbleHMC test that assesses validity of the HMC results for a CAR model.

danielturek commented 8 months ago

@paciorek I'm very happy to see this. Thank you.

I'm pushing some (very) minor changes to style/comments.

One minor question, should the line (# 293 of CAR.R):

 M <- rep(-1234, length = N)

instead be:

 M <- rep(-1234L, length = N)

? It seems that without this, some elements of M (corresponding to zero neighbor islands) might persist as double value -1234, and have derivatives tracked. But perhaps this wouldn't cause any downstream problems, I'm not sure.

perrydv commented 8 months ago

This is minor issue @paciorek but I noticed that in RCfunction_core.R, in fxnsNotAllowedInAD, the dcar_normal and dcar_proper entries do not appear to be correctly entered. They have an extra "d". They were correctly entered in distsNotAllowedInAD. This might mean that they were error-trapped in one place but not another. I am not tracking it down at the moment. And it might become moot from this branch.

paciorek commented 8 months ago

Ok, I've fixed the issue Perry raised. I think it's ok in terms of M. We can't change M to be integer as it contains doubles and setting M in ignore causes a type mismatch.

@perrydv given you responded does means you looked over the changes and things seem fine to you such that I can merge this in?

paciorek commented 8 months ago

Some tests were failing because of typo in numNumeric.

perrydv commented 8 months ago

@paciorek Thanks for this. My previous comment was actually something I just noticed while working on something else, so no I had not done a thorough code review. Now I've looked through it and here are my notes:

In CAR_calcNumIslands

In CAR_calcCmatrix

In the C++

paciorek commented 8 months ago

The failure seems to be unrelated to this PR. I will look into it on devel.