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

Add argument to optionally keep offsets in InferenceData object returned by Model.fit() #288

Merged
merged 6 commits into from Dec 16, 2020

Conversation

tomicapretto
Copy link
Collaborator

@tomicapretto tomicapretto commented Dec 15, 2020

Offset variables were removed from the posterior in #276. However, it may be the case that someone may want to keep them.

This PR adds an argument keep_offsets to Model.fit() which defaults to False.

Checklist

  • Add keep_offsets argument.
  • Fix omit_vars documentation.
  • Add tests.
  • Discuss on existing argument names.

@tomicapretto
Copy link
Collaborator Author

Some thoughts on existing argument names. Currently we have

  • omit_vars in Model.plot_priors(). This omits offsets and group specific terms.
  • omit_offsets in Model.prior_predictive(). This omits offsets.

I am happy with omit_offsets because it clearly indicates its purpose. But I think omit_vars is ambiguous. Also, I think someone would like to plot group specific terms without having to plot offsets. I propose to replace omit_vars in Model.plot_priors() with

  • omit_offsets: omit offsets.
  • omit_group_specific: omit group specific effects.

What are your thoughts on this?

@tomicapretto tomicapretto changed the title [WIP] Add argument to optionally keep offsets in InferenceData object returned by Model.fit() Add argument to optionally keep offsets in InferenceData object returned by Model.fit() Dec 15, 2020
@aloctavodia
Copy link
Collaborator

I agree with split omit_var into two arguments. Also we should unify using "omit" or "keep". Both sound ok to me.

@aloctavodia aloctavodia merged commit ae9a209 into bambinos:master Dec 16, 2020
@tomicapretto tomicapretto deleted the offsets branch December 16, 2020 15:29
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.

None yet

2 participants