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

need to correct IPW code for y and t input format #14

Open
judithabk6 opened this issue Jun 20, 2023 · 6 comments
Open

need to correct IPW code for y and t input format #14

judithabk6 opened this issue Jun 20, 2023 · 6 comments
Assignees

Comments

@judithabk6
Copy link
Owner

IPW implementation behaves badly if t and y dimension is (n, 1), and ok if t and y dimension is (n, 0), please fix

@judithabk6 judithabk6 self-assigned this Jun 20, 2023
@sami6mz
Copy link
Contributor

sami6mz commented Jun 23, 2023

I just realized IPW is not the only one to fail.
g_computation, multiply_robust, simulation_based return ValueError if .ravel() is not applied to t and y.
I don't know about others estimators yet, but I will soon implement a test in response to #15.

@sami6mz
Copy link
Contributor

sami6mz commented Jun 26, 2023

I justed tested .ravel() robustness for all estimators
G_estimator badly behaves too (in addition to IPW, g_computation, multiply_robust, and simulation_based)

@sami6mz
Copy link
Contributor

sami6mz commented Jul 5, 2023

As for multiply_robust, (n,1) format makes the function to fail because some tr variable is not initialised :
l688 - l689

if len(t.shape) == 1:
        tr = t.reshape(-1, 1)

Instead of :

if len(t.shape) == 1:
        tr = t.reshape(-1, 1)
else:
        tr = np.copy(t)
        t = t.ravel()

Just like m.

Besides, why copying t or m is needed?

This was referenced Jul 7, 2023
@sami6mz
Copy link
Contributor

sami6mz commented Jul 7, 2023

Besides, why copying t or m is needed?

Okay I realized get_interactions didn't support (n,) inputs so that's why the copying t and m was needed.
Issue was made in #34, and is to be solved in #35.

@sami6mz
Copy link
Contributor

sami6mz commented Jul 7, 2023

Just a reminder, as for now :

  • IPW returns aberrant indirect effects if not given .ravel inputs
  • g_computation, simulation_based, G_estimator return an error

@judithabk6
Copy link
Owner Author

need to modify the test accordingly when solved, in particular

return data[1].ravel()
and
return data[3].ravel() # same reason as t
(remove the .ravel())

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

No branches or pull requests

2 participants