Skip to content

Generalizing Closeness centrality to weighted networks using Newman method#1385

Merged
IvanIsCoding merged 9 commits intoQiskit:mainfrom
FedericoBruzzone:closeness-wgraph
Feb 14, 2025
Merged

Generalizing Closeness centrality to weighted networks using Newman method#1385
IvanIsCoding merged 9 commits intoQiskit:mainfrom
FedericoBruzzone:closeness-wgraph

Conversation

@FedericoBruzzone
Copy link
Copy Markdown
Contributor

@FedericoBruzzone FedericoBruzzone commented Feb 11, 2025 •
edited
Loading

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.

@FedericoBruzzone
Add Closeness centrality to weighted networks using newman method
1a72203 
Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 11, 2025 •
edited
Loading

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
IvanIsCoding
IvanIsCoding reviewed Feb 12, 2025
View reviewed changes
Copy link
Copy Markdown
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

@FedericoBruzzone
Copy link
Copy Markdown
Contributor Author

FedericoBruzzone commented Feb 12, 2025

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
FedericoBruzzone commented Feb 12, 2025
View reviewed changes
@FedericoBruzzone
Copy link
Copy Markdown
Contributor Author

FedericoBruzzone commented Feb 12, 2025 •
edited
Loading

@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
Copy Markdown
Collaborator

IvanIsCoding 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.

@FedericoBruzzone
Copy link
Copy Markdown
Contributor Author

FedericoBruzzone commented Feb 12, 2025 •
edited
Loading

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
Copy Markdown

coveralls commented Feb 13, 2025 •
edited
Loading

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

IvanIsCoding
IvanIsCoding reviewed Feb 13, 2025
View reviewed changes
Copy link
Copy Markdown
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

IvanIsCoding
IvanIsCoding reviewed Feb 13, 2025
View reviewed changes
@FedericoBruzzone FedericoBruzzone force-pushed the closeness-wgraph branch 2 times, most recently from c344424 to 3fc34ac Compare February 13, 2025 08:05
@FedericoBruzzone
Test by using assert_almost_equal!
86d80aa 
Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>
@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
Copy Markdown
Contributor Author

FedericoBruzzone commented Feb 13, 2025

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
Copy Markdown
Collaborator

IvanIsCoding commented Feb 13, 2025

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.

IvanIsCoding
IvanIsCoding reviewed Feb 13, 2025
View reviewed changes
Copy link
Copy Markdown
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

@FedericoBruzzone
Make available weighted_closeness for python users
a7f6007 
Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>
@FedericoBruzzone
Fix typo in some files
dbe8243 
Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>
@FedericoBruzzone
Add docs and update pyi files
6836056 
Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>
@FedericoBruzzone
Minor fix
865b498 
Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>
@FedericoBruzzone
Copy link
Copy Markdown
Contributor Author

FedericoBruzzone commented Feb 13, 2025 •
edited
Loading

We should be good to go 😄

@FedericoBruzzone
Add releasenote
e72acb5 
Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>
IvanIsCoding
IvanIsCoding reviewed Feb 14, 2025
View reviewed changes
IvanIsCoding
IvanIsCoding approved these changes Feb 14, 2025
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

FedericoBruzzone commented Feb 14, 2025

This is good to go

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

@IvanIsCoding IvanIsCoding mentioned this pull request Feb 14, 2025
Open
@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
@IvanIsCoding IvanIsCoding mentioned this pull request Feb 28, 2025
Merged
SILIZ4 pushed a commit to SILIZ4/rustworkx that referenced this pull request Jul 4, 2025
@FedericoBruzzone @IvanIsCoding @SILIZ4
Generalizing Closeness centrality to weighted networks using Newman…
fedec91 
… method (Qiskit#1385)

* Add `Closeness` centrality to weighted networks using newman method

Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

* Test by using `assert_almost_equal!`

Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

* Make available `weighted_closeness` for python users

Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

* Fix typo in some files

Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

* Add docs and update `pyi` files

Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

* Minor fix

Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

* Add `releasenote`

Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>

* Update rustworkx-core/src/centrality.rs

---------

Signed-off-by: FedericoBruzzone <federico.bruzzone.i@gmail.com>
Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@IvanIsCoding IvanIsCoding IvanIsCoding approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.17.0

Development

Successfully merging this pull request may close these issues.

Generalizing Closeness centrality to weighted networks

4 participants

@FedericoBruzzone @CLAassistant @IvanIsCoding @coveralls