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

Update dowhy_mediation_analysis.ipynb #498

Merged
merged 2 commits into from
Jul 15, 2022
Merged

Conversation

itsoum
Copy link
Contributor

@itsoum itsoum commented Jun 30, 2022

You had assign in causal_estimate_nie variable the natural direct effect and vice versa, so I corrected the typo.

You had assign in causal_estimate_nie variable the natural direct effect and vice versa, so I corrected the typo.
Copy link
Member

@amit-sharma amit-sharma left a comment

Choose a reason for hiding this comment

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

ah yes, thanks for catching this.

Copy link
Member

@amit-sharma amit-sharma left a comment

Choose a reason for hiding this comment

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

@itsoum I noticed that there is an error when running the notebook. Perhaps there is one more name change required?

NameError Traceback (most recent call last) Input In [7], in <cell line: 1>() ----> 1print(causal_estimate_nde.value, data["ate"]) NameError: name 'causal_estimate_nde' is not defined NameError: name 'causal_estimate_nde' is not defined

@itsoum
Copy link
Contributor Author

itsoum commented Jul 7, 2022

@amit-sharma you are right! Done!

@itsoum itsoum requested a review from amit-sharma July 7, 2022 11:46
@itsoum
Copy link
Contributor Author

itsoum commented Jul 14, 2022

@amit-sharma the PR blocked because of lack of proper sign-off by me? Is there any easy and proper way to solve it?
If not I think a simple but unorthodox solution is that I will close this PR without merging and I will resend a PR with the proper sign-off.
What do you say?

@bloebp
Copy link
Member

bloebp commented Jul 14, 2022

To solve the DCO issue, you can put all changes into one commit and update this PR. For instance, make a soft reset, take all changes together and make one new commit with git commit -s the -s will sign off the commit. Then, simply override the commit history of this PR via git push --force-with-lease.

@itsoum
Copy link
Contributor Author

itsoum commented Jul 14, 2022

@bloebp I have done the commits given their simplicity by the GitHub GUI. Is any way to solve the DCO issue by the GitHub GUI or I should fetch it local in my laptop and push it?

@bloebp
Copy link
Member

bloebp commented Jul 14, 2022

Except by overriding the commit history of this branch and adding the missing sign offs, I don't know. Strictly speaking, they should be in there before any code is pushed.
Tagging @petergtz, maybe he has another idea.

@amit-sharma
Copy link
Member

I've approved the DCO for this PR. All good for the merge---thanks @itsoum

@amit-sharma
Copy link
Member

@all-contributors please add @itsoum for code.

@allcontributors
Copy link
Contributor

@amit-sharma

I've put up a pull request to add @itsoum! 🎉

@amit-sharma amit-sharma merged commit 3c96208 into py-why:master Jul 15, 2022
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.

3 participants