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

fix dowhy_ not existance issue #929

Merged
merged 6 commits into from
Dec 5, 2024
Merged

Conversation

louis-she
Copy link
Contributor

  1. object has no __getattr__ attribute so calling super().__getattr__ is wrong, should change it to getattr(self, name).
  2. dowhy_ is not exist until fit is called, just have a more intuitive error message when someone calls methods without fitting.

Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I have two requests:

  1. This repo requires all commits to be signed off (which yours isn't), which is causing the DCO check to fail - please amend your commit to sign off on it (there are detailed instructions that you can see if you look at the details of the failing DCO check).
  2. Please add a test case that fails with the current version of the code but passes with your changes.

@louis-she
Copy link
Contributor Author

if without this patch,

LinearDRLearner().dowhy.refute_estimate(method_name="random_common_cause", num_simulations=3)
# or
LinearDRLearner().dowhy._estimator_object

will raise

Traceback (most recent call last):
  File "/Users/louisshe/Downloads/EconML/econml/tests/test_dowhy.py", line 100, in test_dowhy_without_fit
    LinearDRLearner().dowhy.refute_estimate(method_name="random_common_cause", num_simulations=3)
  File "/Users/louisshe/Downloads/EconML/econml/dowhy.py", line 220, in refute_estimate
    return self.dowhy_.refute_estimate(
  File "/Users/louisshe/Downloads/EconML/econml/dowhy.py", line 236, in __getattr__
    return super().__getattr__(attr)
AttributeError: 'super' object has no attribute '__getattr__'

super() is object because the defination class DoWhyWrapper. And object does not has __getattr__.

>>> object.__getattr__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'object' has no attribute '__getattr__'. Did you mean: '__setattr__'?

So this is incorrect without a doult, instead we should use getattr here, and to prevent infinite recursion, should add a check before calling getattr or hasattr in __getattr__.


All the cases will only happen when use in a wrong way, "accessing attributes without calling fit", so i just added some more intuitive error message comparing the old error message AttributeError: 'super' object has no attribute '__getattr__' .

pre-commit-ci bot and others added 3 commits November 25, 2024 10:35
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.6.2 → v0.7.3](astral-sh/ruff-pre-commit@v0.6.2...v0.7.3)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Keith Battocchi <kebatt@microsoft.com>
Signed-off-by: chenglu <chenglu.she@gmail.com>
Signed-off-by: chenglu <chenglu.she@gmail.com>
Signed-off-by: chenglu <chenglu.she@gmail.com>
@louis-she louis-she requested a review from kbattocchi November 26, 2024 01:58
Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@kbattocchi kbattocchi merged commit 7c35d27 into py-why:main Dec 5, 2024
93 checks passed
@louis-she louis-she deleted the fix-super-getattr branch December 5, 2024 09:10
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.

2 participants