Skip to content

Created base structure for adding discovery and other algorithms #68

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

Merged
merged 10 commits into from
Aug 12, 2025

Conversation

amit-sharma
Copy link
Member

@amit-sharma amit-sharma commented Jul 20, 2025

New files added that convey the directory structure.

This PR does not add any functionality, just the protocol class and structure. The goal is to enable collaborators to contribute code.

@amit-sharma amit-sharma requested a review from Copilot July 20, 2025 07:43
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR establishes the foundational structure for a causal discovery library by creating base interfaces and utility functions. The changes introduce a protocol-based design for datasets and skeleton implementations for evaluation metrics.

  • Defines a Dataset protocol with methods for graph access, data retrieval, and synthetic data generation
  • Creates placeholder functions for standard causal discovery evaluation metrics (accuracy, precision, recall, F1)

Reviewed Changes

Copilot reviewed 2 out of 8 changed files in this pull request and generated 4 comments.

File Description
pywhyllm/datasets/dataset.py Defines the Dataset protocol interface with methods for graph, data, and synthetic data generation
pywhyllm/datasets/metrics.py Creates skeleton functions for evaluation metrics used in causal discovery
Comments suppressed due to low confidence (1)

pywhyllm/datasets/metrics.py:21

  • Function name 'F1' should follow Python naming conventions. Consider renaming to 'f1_score' or 'f1' (lowercase).
def F1(edges, true_edges)

amit-sharma and others added 4 commits July 20, 2025 13:14
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Amit Sharma <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Amit Sharma <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Amit Sharma <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Amit Sharma <[email protected]>
@amit-sharma amit-sharma requested a review from emrekiciman July 21, 2025 04:56
Copy link
Member

@emrekiciman emrekiciman left a comment

Choose a reason for hiding this comment

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

This is great. I added a few questions about the structure.

Also: do we want this dataset protocol to integrate well with data handling in other pywhyllm libraries? Or is this only for self-contained pywhy-llm benchmarking for example?

@amit-sharma
Copy link
Member Author

amit-sharma commented Aug 5, 2025

This is great. I added a few questions about the structure.

Also: do we want this dataset protocol to integrate well with data handling in other pywhyllm libraries? Or is this only for self-contained pywhy-llm benchmarking for example?

thanks for the feedback, @emrekiciman . Currently, dowhy expects a pandas dataframe and a Networkx graph (separately). This dataset class is pywhyllm-specific but can be easily used to provide input to dowhy by accessing its members, graph and data. EconML expects numpy arrays, so there isn't a common format yet.

Addressed other comments inline.
EDIT: I also had to update the pyproject due to some dependency installation issues that broke the build.

Copy link
Member

@emrekiciman emrekiciman left a comment

Choose a reason for hiding this comment

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

lgtm

@emrekiciman emrekiciman merged commit fdbbc37 into main Aug 12, 2025
4 checks passed
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