vast-lib / tinyVAST

Expressive interface for multivariate spatio-temporal models
https://vast-lib.github.io/tinyVAST/
GNU General Public License v3.0
10 stars 2 forks source link

`t2()` or `te()` smoothers crash the model, `s(x, by =)` errors #9

Open seananderson opened 8 months ago

seananderson commented 8 months ago

I added some stops https://github.com/James-Thorson-NOAA/tinyVAST/blob/5617851485c4aa96dc06d40e0b18428c30ce8eb9/R/fit.R#L356-L361

and it's also fine to just leave it like that—I'm just noting it here so it doesn't get forgotten. I might be able to tackle this at some point. There may be other illegal mgcv things the user will do that we'll find... and in my experience people will try to smash anything in here that mgcv can do and quickly let you know it doesn't work.

Also s(x, by =) errors. See unit tests in test-smoothers.R.

seananderson commented 8 months ago

I went down the rabbit hole of getting te()/t2() to work. I thought I had it, but the likelihood function comes back as NA, so I must be doing something dumb still. Regardless, s(x1, x2) works, so it's probably fine to not prioritize this.

Just so I don't lose my train of thought, here's where I was at, in a branch: https://github.com/James-Thorson-NOAA/tinyVAST/commit/3413898cdebf2044711757a8f0a767556800d8b9

This is for te(), which I think is simplest if not using the gamm4-type approach (which uses t2()).

The trick is that gam_setup$smooth[[x]]$S has a length of the number of penalization components of the smoother. So, te(x1, x2) has $S of length 2 not 1. Presumably te(x1, x2, x3) has length 3 and so on.

Then, I believe gamma_k needs to have a longer length to match and you need an extra log_lambda for each additional dimension (although that last part just works as is).

Oh! I think I see it. If gamma_k is expanded then Z_ik needs to match. I'll come back to this.

My test:

 set.seed(1)
 dat <- mgcv::gamSim(1, n = 400, scale = 2)
 m_m <- mgcv::gam(y ~ t2(x1, x2), data = dat)
 m_s <- sdmTMB::sdmTMB(y ~ t2(x1, x2), data = dat, spatial = "off")
 m_v <- fit(formula = y ~ te(x1, x2), data = dat)
James-Thorson-NOAA commented 8 months ago

I'd certainly be curious to address this in the future, thanks for posting the issue and adding the informative errors!