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

Wiener index is a distance measure #6975

Closed
wants to merge 3 commits into from
Closed

Wiener index is a distance measure #6975

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 2, 2023

The Wiener index is actually a measure of distance.

So moved it from its own category to the Distance measures.

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.

Unfortunately this is a backward incompatible change: any existing user code which accesses the wiener module will break. Therefore, in principle would require a warning cycle; i.e. raising a FutureWarning for access patterns like from networkx.wiener import wiener_index or nx.wiener.wiener_index1.

There are a few things that could be done here. If others agree that this reorganization is worth the churn, then we could go ahead with the warning introduction now. Another option would be to table this for a larger namespace cleanup akin to e.g. what scipy did. If we did that in NetworkX, then once that change went through (after a 2-release warning cycle) then moving the implementation either becomes moot (because the location of it's definition is private) or is non-breaking.

Footnotes

  1. neither of these access patterns is recommended, but they are possible so there is likely code that depends on them.

@ghost
Copy link
Author

ghost commented Oct 2, 2023

I didn't realise about those incompatibilities. Looking forward to hear your opinions about the suggested change.

@rossbar rossbar added the Discussion Issues for discussion and decision making label Oct 12, 2023
@rossbar
Copy link
Contributor

rossbar commented Nov 8, 2023

Personally, I'd vote to handle these types of refactors (i.e. moving where things are defined) wholistically rather than on a case-by-case basis. As you've noticed (cf. #7065), this is not the only instance where there may be an opportunity to refactor the module/package structure. I'd prefer to do so all at once rather than introduce individual warnings cycles for each instance.

@ghost
Copy link
Author

ghost commented Nov 8, 2023

I'm fine with such a general overhaul rather than a case-by-case basis. It is a good idea to keep a list of all those suggested changes @rossbar ?

@dschult
Copy link
Member

dschult commented Nov 8, 2023

Could we create an issue that tracks the different suggestions for refactoring?

If/when that issue gets too long or disjoint, we could convert it into a NXEP with a file in doc/developer/nexps and make changes there via PRs. :)

@ghost ghost closed this Mar 4, 2024
@ghost ghost deleted the wiener_index_distance branch March 4, 2024 08:38
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues for discussion and decision making
Development

Successfully merging this pull request may close these issues.

None yet

2 participants