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

Adding degree centrality function #1145

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheMLEngineer
Copy link

Test :
Screenshot at 2024-03-17 16-08-03

As this is my first pull request on this repo , if any more testing need to be done please let me know. Thanks.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

This still needs quite a bit of work. You can start by writing the tests at https://github.com/Qiskit/rustworkx/blob/main/tests/graph/test_centrality.py, the current tests pass because you never called the function you added

///
/// Returns:
/// A `Vec<f64>` containing the degree centrality of each node in the graph.
pub fn degree_centrality<G>(graph: G) -> Result<Option<Vec<f64>>, E>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not compile because you never defined E

graph: &digraph::PyDiGraph,
) -> PyResult<CentralityMapping> {
// Convert Python object to Rust type
let graph_rust = match graph.extract::<SomeRustGraphType>() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also will not compile

@TheMLEngineer
Copy link
Author

Hi @IvanIsCoding , Thanks for the review comments , will check and update

@1ucian0 1ucian0 linked an issue May 14, 2024 that may be closed by this pull request
@Paulo-21
Copy link
Contributor

Hello, any news about this feature ?

@IvanIsCoding
Copy link
Collaborator

@Paulo-21 this Pull Request currently does not build. However, contributions are welcome and I believe #1129 would be fairly straightforward if you want to send a PR implementing it

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.

Degree centrality support
4 participants