Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated multilevel modeling to v4 #285

Merged
merged 20 commits into from
May 9, 2022
Merged

Updated multilevel modeling to v4 #285

merged 20 commits into from
May 9, 2022

Conversation

fonnesbeck
Copy link
Member

@fonnesbeck fonnesbeck commented Feb 27, 2022

Fixed models to work in v4.0.0b2 and added missing remote images as local files.

I don't like the new formulation of the early models as having independent means for floor and basement, but I can tackle that issue in a separate PR.

This should supersede #282

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 27, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-02-27T21:59:59Z
----------------------------------------------------------------

The date should be updated too.


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 27, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-02-27T22:00:00Z
----------------------------------------------------------------

Line #1.    coords = {"Level": ["Basement", "Floor"], "obs_id": np.arange(floor.size)}

There is no need anymore for "decoy" coordinates. Using obs_id for the first time in floor_idx will define it. The default coordinate values are an arange, so the output should not change. It should also use the mutable argument or one of the Constant/Mutable data classes to get rid of the warning.


fonnesbeck commented on 2022-02-27T22:29:12Z
----------------------------------------------------------------

Sorry, not sure what you mean by decoy coordinates. Do they not have to be in the coords dict to be used by floor_idx?

OriolAbril commented on 2022-02-27T22:47:13Z
----------------------------------------------------------------

no, floor_idx is already an array of the right shape, so dims here have only 2 functions (out of 3 possible functions): annotating the dimension, and checking the shape matches for all objects annotated with that dimension. It is not being used to define the shape like it would in pm.Normal("var", 0, 1, dims="obs_id").

With the improved Aesara capabilities and the refactors to shape/size/dims functionality done by Brandon and Michael (I think it was mostly the two of them) when annotating dimensions, we can use dimension names that are not in coords which is very convenient when used in conjunction with pm.Data because the dimension and the variable where it is first annotated are linked so when generating predictions we only need to update the data instead of both data and coordinate values. See for example https://github.com/pymc-devs/pymc/blob/main/pymc/tests/test_shape_handling.py#L336

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 27, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-02-27T22:00:01Z
----------------------------------------------------------------

use constant/mutable things to get rid of warning.


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 27, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-02-27T22:00:01Z
----------------------------------------------------------------

Line #8.        sd_dist = pm.Exponential.dist(0.5, shape=2)

Can this be dims="param" so it's clear for readers what each position corresponds to? I'm not sure if dims work with .dist


fonnesbeck commented on 2022-02-27T22:32:40Z
----------------------------------------------------------------

Unfortunately it is not compatible.

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 27, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-02-27T22:00:02Z
----------------------------------------------------------------

This model doesn't look converged :/


cfonnesbeck commented on 2022-03-04T01:19:01Z
----------------------------------------------------------------

Increased tuning seems to fix it.

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 27, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-02-27T22:00:03Z
----------------------------------------------------------------

Adding predictions to the predictions group can be simplified to

pm.sample_posterior_predictive(contextual_effect_trace, predictions=True, extend_inferencedata=True);


fonnesbeck commented on 2022-02-27T22:42:11Z
----------------------------------------------------------------

Unfortunately this does not seem to work. A posterior_predictive group does not get added does not get added to the trace and has to be added manually.

OriolAbril commented on 2022-02-27T22:49:42Z
----------------------------------------------------------------

this should add a predictions group, it seems to be working on the example in the docs: https://docs.pymc.io/en/latest/learn/examples/posterior_predictive.html#prediction. Which groups do you have?

cfonnesbeck commented on 2022-03-04T01:40:04Z
----------------------------------------------------------------

OK, figured it out.

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 27, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-02-27T22:00:04Z
----------------------------------------------------------------

you should also add yourself again, make sure the notebook is executed sequentially until the end and add the page footer cell


Copy link
Member Author

Sorry, not sure what you mean by decoy coordinates. Do they not have to be in the coords dict to be used by floor_idx?


View entire conversation on ReviewNB

Copy link
Member Author

Unfortunately it is not compatible.


View entire conversation on ReviewNB

Copy link
Member Author

Unfortunately this does not seem to work. A posterior_predictive group does not get added does not get added to the trace and has to be added manually.


View entire conversation on ReviewNB

Copy link
Member

no, floor_idx is already an array of the right shape, so dims here have only 2 functions (out of 3 possible functions): annotating the dimension, and checking the shape matches for all objects annotated with that dimension. It is not being used to define the shape like it would in pm.Normal("var", 0, 1, dims="obs_id").

With the improved Aesara capabilities and the refactors to shape/size/dims functionality done by Brandon and Michael (I think it was mostly the two of them) when annotating dimensions, we can use dimension names that are not in coords which is very convenient when used in conjunction with pm.Data because the dimension and the variable where it is first annotated are linked so when generating predictions we only need to update the data instead of both data and coordinate values. See for example https://github.com/pymc-devs/pymc/blob/main/pymc/tests/test_shape_handling.py#L336


View entire conversation on ReviewNB

Copy link
Member

this should add a predictions group, it seems to be working on the example in the docs: https://docs.pymc.io/en/latest/learn/examples/posterior_predictive.html#prediction. Which groups do you have?


View entire conversation on ReviewNB

Copy link

ghost commented Mar 4, 2022

Increased tuning seems to fix it.


View entire conversation on ReviewNB

Copy link

ghost commented Mar 4, 2022

OK, figured it out.


View entire conversation on ReviewNB

@pymc-devs pymc-devs deleted a comment Mar 4, 2022
@fonnesbeck
Copy link
Member Author

OK, should be good to go now, unless there are further issues.

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 4, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-03-04T19:43:41Z
----------------------------------------------------------------

can you use the mutable kwarg to get rid of the warning?


@review-notebook-app
Copy link

review-notebook-app bot commented Mar 4, 2022

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2022-03-04T19:43:42Z
----------------------------------------------------------------

I would be nice to update the coordinate values here now that obs_id is no longer an index but maps directly to counties. I looked at the docstrings and it seems that only model.set_data accepts coords to be updated, but pm.set_data doesn't.


fonnesbeck commented on 2022-04-11T02:24:05Z
----------------------------------------------------------------

Is there anything we can do about this now?

OriolAbril commented on 2022-04-11T21:27:45Z
----------------------------------------------------------------

use the model.set_data method two times or speed https://github.com/pymc-devs/pymc/pull/5588 up

cfonnesbeck commented on 2022-04-12T00:01:55Z
----------------------------------------------------------------

Let's punt on this until set_data is updated.

@OriolAbril
Copy link
Member

/pre-commit run

Trying to see if this fixes the ci job, you'll have to pull before doing the changes.

@OriolAbril
Copy link
Member

I never can get pre-commit magic comment to work 🤷

Copy link
Member Author

Is there anything we can do about this now?


View entire conversation on ReviewNB

Copy link
Member

use the model.set_data method two times or speed https://github.com/pymc-devs/pymc/pull/5588 up


View entire conversation on ReviewNB

Copy link

ghost commented Apr 12, 2022

Let's punt on this until set_data is updated.


View entire conversation on ReviewNB

ricardoV94 and others added 9 commits May 6, 2022 20:11
* Add guide on how to wrap a JAX function in a Aesara Op

* Fix typos and reorder last sections

* Fix reference paths

* Simplify model and be more verbose in Op creation

* Address review comments

* Address more review comments

* Fix more Aesara docs references

* Use reference to Aesara index page

* Update myst_nbs/case_studies/wrapping_jax_function.myst.md

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
* create truncated regression example

* delete truncated regression example from main branch

* create truncated regression example

* delete truncated regression example from main branch

* create truncated regression example

* delete truncated regression example from main branch

* initial commit

* update link to pull request in Authors section

* add tag `classification`

* update authorship verbs

* plot through xarray, using XrContinuousRV

* add x axis labels

* add xarray_einstats to watermark, and fix 'classification' as a tag, not a category

Co-authored-by: Benjamin T. Vincent <drbenvincent@users.noreply.github.com>
Add *.DS_Store to .gitignore

Co-authored-by: Benjamin T. Vincent <drbenvincent@users.noreply.github.com>
* create truncated regression example

* delete truncated regression example from main branch

* create truncated regression example

* delete truncated regression example from main branch

* create truncated regression example

* delete truncated regression example from main branch

* initial commit

* obey the law in terms of numpy random number generation + az style

* improvements to schematic figure

* replace image with non transparent background

* fix embedded image

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* fix embedded image - not sure why this didn't register before

* address all review comments

- change notebook tag
- remove plt.tight_layout()
- ax.legend() -> plt.legend()

* make code cell visible

* remove 2 unused lines

* add explanation tag

* add technical note re posterior predictive sampling

* addressing Luciano's suggested edits

* increase clarity of sentence in introduction

* add another explanatory markdown cell on the effect posterior

* fix typo

Co-authored-by: Benjamin T. Vincent <drbenvincent@users.noreply.github.com>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
* initial try

* undo spacing error

* specify location files

* update paths

* add exclude notebooks

* missing notebooks

* undo spacing
* updated notebook

* run wit latest v2 version

* fixed graphviz redering

* partial resolution of pr comments

* Add guide on how to wrap a JAX function in a Aesara Op (#302)

* Add guide on how to wrap a JAX function in a Aesara Op

* Fix typos and reorder last sections

* Fix reference paths

* Simplify model and be more verbose in Op creation

* Address review comments

* Address more review comments

* Fix more Aesara docs references

* Use reference to Aesara index page

* Update myst_nbs/case_studies/wrapping_jax_function.myst.md

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* update twitter link (#314)

* update Gaussian Mixture Model example with `pm.NormalMixture` (#310)

* create truncated regression example

* delete truncated regression example from main branch

* create truncated regression example

* delete truncated regression example from main branch

* create truncated regression example

* delete truncated regression example from main branch

* initial commit

* update link to pull request in Authors section

* add tag `classification`

* update authorship verbs

* plot through xarray, using XrContinuousRV

* add x axis labels

* add xarray_einstats to watermark, and fix 'classification' as a tag, not a category

Co-authored-by: Benjamin T. Vincent <drbenvincent@users.noreply.github.com>

* add `*.DS_Store` to `.gitignore` (#315)

Add *.DS_Store to .gitignore

Co-authored-by: Benjamin T. Vincent <drbenvincent@users.noreply.github.com>

* updated notebook

* run wit latest v2 version

* fixed graphviz redering

* partial resolution of pr comments

* reverted plot labels

* restored plt labels

* fixed arviz labels

* fixed sampling warning

* fixed authors

Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
Co-authored-by: Abhishek K M <67158080+Sync271@users.noreply.github.com>
Co-authored-by: Benjamin T. Vincent <b.t.vincent@dundee.ac.uk>
Co-authored-by: Benjamin T. Vincent <drbenvincent@users.noreply.github.com>
* add bibtex reference for Rasmussen & Williams book

* update the latest Mauna Loa C02 data
@fonnesbeck
Copy link
Member Author

Should be clean now.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented on the myst because it was easier, best to make the changes on from jupyter

.pre-commit-config.yaml Outdated Show resolved Hide resolved
myst_nbs/case_studies/multilevel_modeling.myst.md Outdated Show resolved Hide resolved
@fonnesbeck
Copy link
Member Author

I'm getting an erroneous jupytext failure:

image

The two links in this paragraph are external.

@OriolAbril
Copy link
Member

These should be flagged, it's the check name that is not right, it finds "urls that should be sphinx cross-references"

The one to the arviz homepage should be {doc}`arviz:index` the one to the xarrayforarviz notebook should be {ref}`xarray_for_arviz`

"# A Primer on Bayesian Methods for Multilevel Modeling\n",
"\n",
":::{post} 27 February, 2022\n",
":tags: hierarchical, pymc.Data, pymc.Deterministic, pymc.Exponential, pymc.LKJCholeskyCov, pymc.Model, pymc.MvNormal, pymc.Normal\n",
":tags: hierarchical, Data, Deterministic, Exponential, LKJCholeskyCov, Model, MvNormal, Normal\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
":tags: hierarchical, Data, Deterministic, Exponential, LKJCholeskyCov, Model, MvNormal, Normal\n",
":tags: hierarchical model, case study\n",

@@ -4,10 +4,11 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"(notebook_name)=\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"(notebook_name)=\n",
"(multilevel_modeling)=\n",

myst_nbs/case_studies/multilevel_modeling.myst.md Outdated Show resolved Hide resolved
myst_nbs/case_studies/multilevel_modeling.myst.md Outdated Show resolved Hide resolved
@OriolAbril OriolAbril merged commit bea3ac1 into pymc-devs:main May 9, 2022
@OriolAbril
Copy link
Member

related to #47

@hectormz
Copy link
Contributor

hectormz commented Nov 7, 2023

@fonnesbeck I know this is an old PR, but I was wondering if you made the model pngs added in this PR via latex or some other method? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants