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

Overhauled readme #1088

Merged
merged 7 commits into from
Dec 1, 2023
Merged

Overhauled readme #1088

merged 7 commits into from
Dec 1, 2023

Conversation

bloebp
Copy link
Member

@bloebp bloebp commented Nov 26, 2023

The current readme is too overloaded for users. Changes here are:

  • Reduce it only the essential points, since most of the content is part of the documentation
  • 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
  • Extended some references to include GCM related work
  • Fix build status icon
  • Changed github references to py-why (was still pointing to microsoft)

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:
Screenshot 2023-11-28 at 08 52 50

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.

Added some comments. Let me also make some edits to the readme text and push changes.

A few comments on the image:

  1. 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.
  2. 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.
  3. 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.
  4. 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)"
  5. 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.

README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
bloebp and others added 3 commits November 28, 2023 09:18
- 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>
Signed-off-by: Patrick Bloebaum <bloebp@amazon.com>
@emrekiciman
Copy link
Member

Thanks for this, @bloebp ! The new README is much more streamlined and easier to read. A couple comments:

  • I'd highlight the "documentation is available" link at the top of the README much more strongly. Maybe put it in a highlighted box or a larger font. Also change to "documentation, user guide, and sample notebooks" to make it clear it is more than API docs.

  • Somewhere, we should mention that DoWhy is part of PyWhy and a family of tools, and point to https://pywhy.org/

  • I agree with Amit about the 'optional' language for refutations.

  • I'd suggest titling the whole "installation" and "example usage" together as a "Quick Start" guide. So the whole README becomes 3 major sections: (1) "Intro & key features"; (2) Quick Start; and (3) More information and resources. Not necessarily in this order.

Signed-off-by: Patrick Bloebaum <bloebp@amazon.com>
@bloebp bloebp force-pushed the revise_readme branch 4 times, most recently from 8f6298e to e0dea67 Compare November 28, 2023 20:05
Signed-off-by: Patrick Bloebaum <bloebp@amazon.com>
@bloebp
Copy link
Member Author

bloebp commented Nov 28, 2023

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:
https://github.com/py-why/dowhy/blob/c0ac8433625b5693175629076ad32a92b0d1eb71/README.rst

and the new image is:
Screenshot 2023-11-28 at 08 52 50

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>
Signed-off-by: Patrick Bloebaum <bloebp@amazon.com>
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.

thanks for the changes @bloebp . Approving.

@bloebp bloebp merged commit f455c56 into main Dec 1, 2023
2 checks passed
@bloebp bloebp deleted the revise_readme branch December 1, 2023 14:58
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