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

Undocumented parameters in dispersion #6183

Merged
merged 15 commits into from
Nov 8, 2022
Merged

Undocumented parameters in dispersion #6183

merged 15 commits into from
Nov 8, 2022

Conversation

Qudirah
Copy link
Contributor

@Qudirah Qudirah commented Nov 7, 2022

fixes for #6111

@Qudirah
Copy link
Contributor Author

Qudirah commented Nov 7, 2022

@rossbar What do you think?

Comment on lines 23 to 24
If normalized is True (default), try out different values of alpha, b and, c to obtain maximum
performance of normalized dispersion.
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I think the best thing to do would be to explicitly describe how the parameters are used in the embeddedness normalization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

" To strengthen the performance of a normalized dispersion, the nodes can be ranked by the order of the function, (disp(u, v) + b)α/(emb(u, v) + c). Searching over choices of alpha, b, and, c leads to maximum performance." How about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and pushed up a suggested wording which includes the calculation explicitly as you've proposed (with rst formatting).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. Thanks for your help!

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this @Qudirah , LGTM!

@MridulS MridulS merged commit 1d3b274 into networkx:main Nov 8, 2022
@MridulS
Copy link
Member

MridulS commented Nov 8, 2022

Thanks @Qudirah!

@Qudirah Qudirah deleted the bugfix-for-6111 branch November 8, 2022 20:19
@Qudirah
Copy link
Contributor Author

Qudirah commented Nov 8, 2022

Thanks @Qudirah!

:)

@jarrodmillman jarrodmillman added this to the networkx-3.0 milestone Nov 10, 2022
MridulS pushed a commit to MridulS/networkx that referenced this pull request Feb 4, 2023
* fixes networkx#6036

* test load centrality

* test dispersion

* test dispersion

* dispersion test

* test dispersion

* bug-fixes-for-issue-6088

* deleted

* bugfix for 6111

* Add suggestion for parameter description.

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Mjh9122 pushed a commit to Mjh9122/networkx that referenced this pull request Feb 27, 2023
* fixes networkx#6036

* test load centrality

* test dispersion

* test dispersion

* dispersion test

* test dispersion

* bug-fixes-for-issue-6088

* deleted

* bugfix for 6111

* Add suggestion for parameter description.

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* fixes networkx#6036

* test load centrality

* test dispersion

* test dispersion

* dispersion test

* test dispersion

* bug-fixes-for-issue-6088

* deleted

* bugfix for 6111

* Add suggestion for parameter description.

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants