pymc-devs / pymc-examples

Examples of PyMC models, including a library of Jupyter notebooks.
https://www.pymc.io/projects/examples/en/latest/
MIT License
259 stars 212 forks source link

Conditional autoregressive priors #547

Closed daniel-saunders-phil closed 1 year ago

daniel-saunders-phil commented 1 year ago

417 is got stuck and the original author isn't pursing the PR anymore. However, it would be nice to get this notebook finished. I just don't have the permissions to change #417 directly. So I've made a new PR to, at the very least, test out the preview of read the docs and see if it passes continuous integration checks on github.

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

daniel-saunders-phil commented 1 year ago

What's the right technique to generate and then fix these merge conflicts locally?

daniel-saunders-phil commented 1 year ago

I've managed to pass all the pre-commit checks locally, so that's progress for this notebook. I ended up finding a half dozen other typos that were causing pre-commit to fail. However, the preview file is rendered terribly plus the branch conflicts. Not sure how to fix those yet.

$ git commit -m "changed aesara.tensor to pytensor.tensor"
jupytext......................................................................................Passed
black-jupyter.................................................................................Passed
nbqa-isort....................................................................................Passed
nbqa-pyupgrade................................................................................Passed
Check cells were executed sequentially........................................................Passed
bibtex-tidy...............................................................(no files to check)Skipped
Check notebooks have watermark (see Jupyter style guide from PyMC docs).......................Passed
Check no internal links are in the docs.......................................................Passed
jupytext......................................................................................Passed
codespell.....................................................................................Passed
[conditional_autoregressive_priors 4d2741d] changed aesara.tensor to pytensor.tensor
 2 files changed, 346 insertions(+), 17 deletions(-)
daniel-saunders-phil commented 1 year ago

I think it might be done! @bwengals and I fought through a lot of pre-commit to get here 🫠

daniel-saunders-phil commented 1 year ago
  1. I am completely lost on what is the relation between this notebook and the existing one. Are they complementary, should this replace the other one, is the old one still relevant and salvageable (even if this one is somewhat complementary)?

I think this notebook replaces the other one for most users. The old CAR notebook is great for explaining why PyMC's car prior is implemented the way it is. So users who are interested in understanding how to write and modify their own algorithms for similar models autoregressive models would benefit from reading the old notebook (albeit, with up dates to pymc 5). For everyone else, the new notebook is more useful - it explain a bit about why we should care about spatial autocorrelation and is far more concise.

OriolAbril commented 1 year ago

I think this notebook replaces the other one for most users. The old CAR notebook is great for explaining why PyMC's car prior is implemented the way it is. So users who are interested in understanding how to write and modify their own algorithms for similar models autoregressive models would benefit from reading the old notebook (albeit, with up dates to pymc 5). For everyone else, the new notebook is more useful - it explain a bit about why we should care about spatial autocorrelation and is far more concise.

Thanks, this is great!

Would you be interested in updating the other one too? If so we can continue the discussion once the time comes, otherwise (or in the meantime if it is expected to be a longish wait), what do you think about updating the title of the other notebook to be "About CAR models in PyMC"? I think it is more fitting given your description

daniel-saunders-phil commented 1 year ago

Yeah I'd like to revise the other one at some point. I think I'll get to it after my next task (trying to develop the ICAR) so an interim title change sounds good. Would I update the (conditional_autoregressive_model)= at the top of the file plus the references to the title in my current notebook?

A quick explanation of the other changes to the file: previously the notebook suggested model 3 was unidentifiable and trying to sample from it would typically result in chains with loads of divergences. I wasn't able to reliably reproduce these divergences and can't find evidence of degenerate geometries in the posterior distribution. I think the divergences discussed in the earlier drafts might be due to bad luck. To avoid discussion of divergences, I rewrote the bottom few cells to provide a different motivation for the ICAR.

OriolAbril commented 1 year ago

Would I update the (conditional_autoregressive_model)= at the top of the file plus the references to the title in my current notebook?

The main thing that needs to be updated is the title, the # Conditional Autoregressive (CAR) model. The ()= part is a target/anchor. It is something to be used for cross-referencing, and doesn't appear anywhere on the rendered website. As it is brand new and we can be sure no notebooks are using it, it can be updated too. Otherwise, it'd be best to add two targets, so we don't need to search for references to that in all notebooks and modify them (with potential git conflicts...). We already do that in https://github.com/pymc-devs/pymc-examples/blob/main/examples/case_studies/multilevel_modeling.ipynb for example

daniel-saunders-phil commented 1 year ago

Hi @OriolAbril I think it might be ready to go again - I adjusted the metadata and the title of the old CAR notebook as you suggested. My read the docs preview doesn't seem to be up to date but the committed files are all where I think they should be.

OriolAbril commented 1 year ago

Thanks!