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.

Sign up for GitHub

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

Jump to bottom

Remove InstrumentalVariableRegression.fit method #329

Closed
drbenvincent opened this issue May 6, 2024 · 3 comments · Fixed by #335
Closed

Remove InstrumentalVariableRegression.fit method #329

drbenvincent opened this issue May 6, 2024 · 3 comments · Fixed by #335
Labels
devops DevOps related good first issue Good for newcomers

Comments

@drbenvincent
Copy link
Collaborator

drbenvincent commented May 6, 2024 •
edited
Loading

The InstrumentalVariableRegression.fit method overrides the ModelBuilder.fit method, but from what I can see this is unnecessary. And it actually is behind the curve because it doesn't grab the random_seed and provide it to pm.sample_prior_predictive or pm.sample_posterior_predictive.

Obviously check that local tests pass. As of right now we have some issues with doctests, see #323, and I'm actively working on that.

cc @NathanielF
@drbenvincent drbenvincent added good first issue Good for newcomers devops DevOps related labels May 6, 2024
@anevolbap
Copy link
Contributor

The InstrumentalVariableRegression.fit method overrides ModelBuilder.fit to accommodate the Z dataframe, which is also necessary for the build_model method. Perhaps it would be simpler to just include the random_seed.

@drbenvincent
Copy link
Collaborator Author

Ah I checked to quickly and missed that. Yes, in that case we just need the random seed in there.

@anevolbap anevolbap mentioned this issue May 6, 2024
Merged
@anevolbap
Copy link
Contributor

Ah I checked to quickly and missed that. Yes, in that case we just need the random seed in there.

Done here.

drbenvincent added a commit that referenced this issue May 7, 2024
@drbenvincent
Merge pull request #335 from anevolbap/issue_329-IV-fit-method
0ffe348 
Fix #329: Add `random_seed` to the `fit` method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
devops DevOps related good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #329: Add random_seed to the InstrumentalVariableRegression.fit method. anevolbap/CausalPy
2 participants
@drbenvincent @anevolbap