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

refactor(structure): restructure code so we can properly support many… #18

Merged
merged 6 commits into from
Jan 2, 2024

Conversation

LilithWittmann
Copy link
Collaborator

… algorithms

Copy link
Member

@this-is-sofia this-is-sofia left a comment

Choose a reason for hiding this comment

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

Independence Tests and Orientations Tests are relabelled as Pipeline Steps which makes the code more readable. Relicts are removed and the file structure is refined. Only some doc strings should be formulated differently for clarity.


class AbstractGraphModel(GraphModelInterface, ABC):
"""
The graph model is the main class of causy. It is responsible for creating a graph from data and executing the pipeline_steps.
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean: The graph model is the main class of causy. It is responsible for initialising nodes for each variable that we have data from, then initializing a fully connected graph by adding edges between each pair of these nodes and then executing the pipeline_steps.

The graph model is the main class of causy. It is responsible for creating a graph from data and executing the pipeline_steps.

The graph model is responsible for the following tasks:
- Create a graph from data (create_graph_from_data)
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member

Choose a reason for hiding this comment

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

"create nodes for each variabel" or "initialise a fully connected graph"

"""
Take the actions returned by the test

In causy changes on the graph are not executed directly. Instead, the test returns an action which should be executed on the graph.
Copy link
Member

Choose a reason for hiding this comment

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

test -> independence test or orientation rule

Take the actions returned by the test

In causy changes on the graph are not executed directly. Instead, the test returns an action which should be executed on the graph.
This is done to make it possible to execute the tests in parallel as well as to decide proactively at which point in the decisions taken by the pipeline step should be executed.
Copy link
Member

Choose a reason for hiding this comment

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

"at which part in the decision": minus in I think


In causy changes on the graph are not executed directly. Instead, the test returns an action which should be executed on the graph.
This is done to make it possible to execute the tests in parallel as well as to decide proactively at which point in the decisions taken by the pipeline step should be executed.
Actions are returned by the test and are executed on the graph. The actions are stored in the action history to make it possible to revert the actions or use them in a later step.
Copy link
Member

Choose a reason for hiding this comment

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

test -> pipeline step? independence test or orientation rule? (it's not only tests)

This is done to make it possible to execute the tests in parallel as well as to decide proactively at which point in the decisions taken by the pipeline step should be executed.
Actions are returned by the test and are executed on the graph. The actions are stored in the action history to make it possible to revert the actions or use them in a later step.

:param results:
Copy link
Member

Choose a reason for hiding this comment

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

the results of the tests and rules

Actions are returned by the test and are executed on the graph. The actions are stored in the action history to make it possible to revert the actions or use them in a later step.

:param results:
:return:
Copy link
Member

Choose a reason for hiding this comment

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

the action on the graph


def execute_pipeline_step(self, test_fn: PipelineStepInterface):
"""
Execute a single pipeline_step on the graph. either in parallel or in a single process depending on the test_fn.parallel flag
Copy link
Member

Choose a reason for hiding this comment

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

either -> Either

@this-is-sofia this-is-sofia merged commit bcb624f into main Jan 2, 2024
8 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