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

Clean up libgalois algorithms and add TODOs for notable issues. #171

Closed

Conversation

arthurp
Copy link
Contributor

@arthurp arthurp commented Apr 21, 2021

No description provided.

@arthurp arthurp requested a review from gurbinder533 April 21, 2021 21:08
Comment on lines +484 to +491
// TODO(amp): This is incorrect. For Node2vec this needs to be:
// Algorithm::Graph::Make(pg, {}, {}) // Ignoring all properties.
// For Edge2vec this needs to be:
// Algorithm::Graph::Make(pg, {}, {edge_type_property_name})
// The current version requires the input to have exactly the properties
// expected by the algorithm implementation.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue needs to be addressed to wrap RandomWalks. The algorithm will currently not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. We need to separate them out into different applications.

@arthurp
Copy link
Contributor Author

arthurp commented Apr 21, 2021

There are a couple of smaller issues that I would like to address before wrapping:

// TODO(amp): This algorithm defines the semantics of the call. If there were
// an algorithm that, for instance, took a list of edges, that would need to
// be a different function, not just a different plan, since it takes
// semantically different arguments. I do think this should have a plan, even
// if there is only one concrete algorithm, but it should be defined and
// documented in terms of the concrete algorithm, not the semantics of the
// function (which is described well below).

// TODO(amp): The parameters walk_length, number_of_walks,
// backward_probability, and forward_probability control the expected output,
// not the algorithm used to compute the output. So they need to be parameters
// on the algorithm function, not in the plan. The plan should be parameters
// which do not change the expected output (though they may cause selecting a
// different correct output).

I can convert all these into Github and/or JIRA issues if that's better for you. This was just a convenient way to communicate this for me at the moment.

@arthurp arthurp force-pushed the feature/clean-up-algorithms branch from e62ff06 to 05e9bb0 Compare April 21, 2021 21:20
@arthurp
Copy link
Contributor Author

arthurp commented Apr 21, 2021

There are other TODOs in this PR, but they are mostly documentation issues, so they do not directly affect wrapping.

@arthurp arthurp force-pushed the feature/clean-up-algorithms branch from 05e9bb0 to e9a96f7 Compare April 21, 2021 21:50
@arthurp
Copy link
Contributor Author

arthurp commented Apr 25, 2021

Closing this in favor of #177. This commit is in that PR and some of the issues are addressed there in one way or another.

@arthurp arthurp closed this Apr 25, 2021
@arthurp arthurp deleted the feature/clean-up-algorithms branch June 17, 2021 14:29
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