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

Generalizing Closeness centrality to weighted networks using Newman method #1385

Merged
merged 9 commits into from
Feb 14, 2025

Conversation

FedericoBruzzone
Copy link
Contributor

@FedericoBruzzone FedericoBruzzone commented Feb 11, 2025

Close #1384

  • I ran rustfmt locally
  • I have added the tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@CLAassistant
Copy link

CLAassistant commented Feb 11, 2025

CLA assistant check
All committers have signed the CLA.

@FedericoBruzzone FedericoBruzzone changed the title Generalizing Closeness centrality to weighted networks using newman method Generalizing Closeness centrality to weighted networks using Newman method Feb 11, 2025
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.

THe implemenation LGTM for rustworkx-core, do you intend to make the method available for Pytohn users though?

I left a comment about the tests. It applies to all of our centrality metrics, we need to be careful with floats

rustworkx-core/src/centrality.rs Outdated Show resolved Hide resolved
@FedericoBruzzone
Copy link
Contributor Author

THe implemenation LGTM for rustworkx-core, do you intend to make the method available for Pytohn users though?

I left a comment about the tests. It applies to all of our centrality metrics, we need to be careful with floats

Thanks for the review. I do not actually need the python implementation, but I can understand that this could improve usability for Python users. So, I will work on exposing the method to Python as well.

Regarding the tests, I appreciate the feedback. I'll make sure to add checks that account for floating-point precision issues across all centrality metrics.

@FedericoBruzzone
Copy link
Contributor Author

FedericoBruzzone commented Feb 12, 2025

@IvanIsCoding Make sure that the following commit is in line with the rest, but most importantly, make sure it is correct. Make available weighted_closeness for python users

Why is CI not starting? 🤔

@IvanIsCoding
Copy link
Collaborator

I need to manually approve CI.

Also, I asked about Python just to confirm. Someone else can work on the Python bindings.

@FedericoBruzzone
Copy link
Contributor Author

FedericoBruzzone commented Feb 12, 2025

I need to manually approve CI.

Also, I asked about Python just to confirm. Someone else can work on the Python bindings.

There was no problem to implement it. It was a pleasure, I look forward to your review :D

@IvanIsCoding IvanIsCoding added this to the 0.17.0 milestone Feb 13, 2025
@coveralls
Copy link

coveralls commented Feb 13, 2025

Pull Request Test Coverage Report for Build 13320423785

Details

  • 4 of 108 (3.7%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 95.313%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rustworkx-core/src/centrality.rs 0 48 0.0%
src/centrality.rs 0 56 0.0%
Totals Coverage Status
Change from base Build 13300446603: -0.5%
Covered Lines: 18425
Relevant Lines: 19331

💛 - Coveralls

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.

Just some minor details.

Also, let me know if you want to work on Python tests & the release note. I can do it if you want to focus on the rustworkx-core core. Adding support for Python are additional extra steps that can be delegated to a separate PR

.gitignore Outdated Show resolved Hide resolved
rustworkx-core/src/centrality.rs Outdated Show resolved Hide resolved
rustworkx-core/src/centrality.rs Outdated Show resolved Hide resolved
rustworkx-core/src/centrality.rs Show resolved Hide resolved
rustworkx-core/src/centrality.rs Outdated Show resolved Hide resolved
rustworkx-core/src/centrality.rs Outdated Show resolved Hide resolved
rustworkx-core/src/centrality.rs Outdated Show resolved Hide resolved
rustworkx-core/src/centrality.rs Outdated Show resolved Hide resolved
rustworkx/rustworkx.pyi Outdated Show resolved Hide resolved
rustworkx/rustworkx.pyi Outdated Show resolved Hide resolved
src/centrality.rs Outdated Show resolved Hide resolved
src/centrality.rs Outdated Show resolved Hide resolved
@FedericoBruzzone FedericoBruzzone force-pushed the closeness-wgraph branch 2 times, most recently from c344424 to 3fc34ac Compare February 13, 2025 08:05
Signed-off-by: FedericoBruzzone <[email protected]>
@FedericoBruzzone FedericoBruzzone force-pushed the closeness-wgraph branch 3 times, most recently from 431bc6f to e1f9418 Compare February 13, 2025 08:29
@FedericoBruzzone
Copy link
Contributor Author

Adding support for Python are additional extra steps that can be delegated to a separate PR

What are the additional steps beyond those made in this PR? Are they documented anywhere? I did not see them in the CONTRIBUTING file. This should be my task and I would like to do it 😄

let me know if you want to work on Python tests & the release note

If you can do that it is better, I will as soon as I can maybe implement the betweeness or the other method for closeness. Just out of curiosity, is it documented anywhere how to do release notes?

@IvanIsCoding
Copy link
Collaborator

The additional steps are compared to the work of adding just Rust code vs adding Python support. For Python:

pip install -U reno
reno new short-description-string

Because we have Rust tests, I am ok with merging the PR without Python tests and adding them later.

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.

Set up the weight_fn to be None by default to agree with the type annotations and we should be good to go. Also, default_weight=1.0 makes the function behave like the unweighted version when no weight function is passed

src/centrality.rs Outdated Show resolved Hide resolved
src/centrality.rs Outdated Show resolved Hide resolved
Signed-off-by: FedericoBruzzone <[email protected]>
@FedericoBruzzone
Copy link
Contributor Author

FedericoBruzzone commented Feb 13, 2025

We should be good to go 😄

Signed-off-by: FedericoBruzzone <[email protected]>
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 is good to go

@FedericoBruzzone
Copy link
Contributor Author

This is good to go

I hope to find more time to do more PRs. Thanks for the review!

@IvanIsCoding IvanIsCoding added this pull request to the merge queue Feb 14, 2025
Merged via the queue into Qiskit:main with commit 286a719 Feb 14, 2025
31 checks passed
@FedericoBruzzone FedericoBruzzone deleted the closeness-wgraph branch February 14, 2025 08:14
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.

Generalizing Closeness centrality to weighted networks
4 participants