Change how NA is handled in bdlim

AnderWilson / bdlim

Code for fitting and interpreting Bayesian distributed lag interaction models (BDLIMs).

https://anderwilson.github.io/bdlim/

GNU General Public License v3.0

0 stars 1 forks source link

Change how NA is handled in bdlim #6

Open hhp94 opened 5 months ago

hhp94 commented 5 months ago

Dr. Wilson,

Currently, NA values are allowed in {bdlim} and handled by list-wise deletion. I propose that we disallow NA values instead, for the following reasons:

G is a factor, and covariates can also contain factors. If a factor level is completely dropped after list-wise deletion, MCMC might not work properly due to the presence of a column of 1s or 0s in the model.matrix.
If random effects are used, a similar problem arises. We might drop a random intercept completely, creating a column of 0s in the random effect model matrix.
Allowing NA values makes the code more fragile. We do not return the list-wise deleted data.frame, so users are unaware of which rows were deleted. I plan to implement {loo} for model validation. In this case, problematic observations flagged by {loo} may not match the original data.

Please let me know what you think

AnderWilson commented 5 months ago

Good idea. That is certainly simpler.

hhp94 commented 4 months ago

Dr. Wilson, may I ask another question regarding our code?

I've noticed that we are currently keeping the burn-in iterations for all parameters. However, as far as I can tell, we don't use these burn-in iterations anywhere else in the code. I believe there are several issues with this approach:

Keeping the burn-in makes the code cumbersome. We have to subset the chains to iterkeep for every parameter in every method (e.g., summary, print, plot).
For large burn-ins and exposures, this can be memory-intensive.
If a user wants the full chain and prefers to subset it themselves, they can simply set nburn to 0.

For these reasons, I think bdlim1 should just return the kept chains. If you agree that this is a good idea, I'd be happy to implement it. Otherwise, I can leave the current behavior as is.

Please let me know your thoughts on this matter.

hhp94 commented 4 months ago

Dear Dr. @AnderWilson,

I've implemented all the changes in #4 as well as other minor fixes and compiled them into a quarto report here: proposal.zip. If you have any comments please let me know. I'll wait for your comments on keeping the burn-ins or not, then I will clean up and send you a zip file of the codes to avoid the merge conflicts. I can also create a PR as well.

Best