-
Notifications
You must be signed in to change notification settings - Fork 938
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
Overhauled readme #1088
Overhauled readme #1088
Conversation
37b8f67
to
16e6121
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments. Let me also make some edits to the readme text and push changes.
A few comments on the image:
- For falsify/refute: rather than optional, shall we say "Suggested checks" or "optional but highly recommended". If we say "optional", people might think that it is not required for a good analysis.
- The Effect estimation actually has sub-tasks within it. How about adding another top-level category (in vertical font) as Effect estimation, then the blocks can be: Average Causal Effect, Conditional Causal Effect (CACE), Direct and Indirect Effects. This will make the figure consistent with the structure in the User Guide.
- Modeling causal relations: Just like we have the causal mechanisms block, can we add a Identify Effect block just after the "define causal graph". It could be a simple block with "Identify causal effect" as the header and the backdoor equation
$P(y|do(x))=...$ as the white inset. - The "define causal graph" block has some empty space. How about adding a sentence, 'Based on domain knowledge or using causal discovery algorithms (e.g., py-why/causal-learn)"
- This is a tentative suggestion: How do you feel about moving out-of-domain prediction to the what-if category? It is the only singleton block in the image, and I feel it does fit in "what-if". OOD prediction can be interpreted as a what-if question after the changing the input features to new values.
- Reduce it only the essential points - Add image as overview of the offered features - Made connection between GCM and PO framework more consistent - Revise the GCM example to an executable code snipped - Removed conda installation guide, because conda is still at version 0.8 - Extended some references to include GCM related work - Fix build status icon - Changed github references to py-why (was still pointing to microsoft) Signed-off-by: Patrick Bloebaum <bloebp@amazon.com>
Signed-off-by: Amit Sharma <amit_sharma@live.com>
Thanks for this, @bloebp ! The new README is much more streamlined and easier to read. A couple comments:
|
a7d061e
to
04f8317
Compare
8f6298e
to
e0dea67
Compare
Signed-off-by: Patrick Bloebaum <bloebp@amazon.com>
e0dea67
to
78bb2fd
Compare
Thanks a lot @amit-sharma @emrekiciman! Somehow, the options in displaying stuff in the README.rst is quite limited (and different from the usual rst files). I hope the new version now this is good compromise to make the documentation link more prominent. The .rst file can be accessed here: PS: The reason why I didn't use the backdoor formula for identification is to avoid the impression that the other features are not relying on that. Let me know what you think |
Signed-off-by: Patrick Bloebaum <bloebp@amazon.com>
87accfd
to
e163df5
Compare
Signed-off-by: Patrick Bloebaum <bloebp@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the changes @bloebp . Approving.
The current readme is too overloaded for users. Changes here are:
Here the new readme: https://github.com/py-why/dowhy/blob/c0ac8433625b5693175629076ad32a92b0d1eb71/README.rst
The image in the key features section is not show. It is the following: