Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.9k
networkx: fix subgraph and degree#14390
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Hmm, |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, remarks below.
| @overload | ||
| def __call__(self, nbunch: _Node, weight: None | bool | str = None) -> int: ... | ||
| @overload | ||
| def __call__(self, nbunch: Iterable[_Node] | None = None, weight: None | bool | str = None) -> DiDegreeView[_Node]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably overly pedantic, but this doesn't necessarily return aDiDegreeView, but could also return a sub-class if called on an instance of a sub-class. SoSelf is more appropriate:
| def__call__(self,nbunch:Iterable[_Node]|None=None,weight:None|bool|str=None)->DiDegreeView[_Node]: ... | |
| def__call__(self,nbunch:Iterable[_Node]|None=None,weight:None|bool|str=None)->Self: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
good point, fixed
| @@ -55,7 +55,10 @@ class NodeDataView(AbstractSet[_Node]): | |||
| class DiDegreeView(Generic[_Node]): | |||
| def __init__(self, G: Graph[_Node], nbunch: _NBunch[_Node] = None, weight: None | bool | str = None) -> None: ... | |||
| def __call__(self, nbunch: _NBunch[_Node] = None, weight: None | bool | str = None) -> int | DiDegreeView[_Node]: ... | |||
| @overload | |||
| def __call__(self, nbunch: _Node, weight: None | bool | str = None) -> int: ... | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
__getitem__ returnsfloat, so this should also returnfloat:
| def__call__(self,nbunch:_Node,weight:None|bool|str=None)->int: ... | |
| def__call__(self,nbunch:_Node,weight:None|bool|str=None)->float: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
hmm shouldn't the degree always be an integer, at least this passes:
importnetworkxasnxg=nx.Graph()g.add_node('a')asserttype(g.degree('a'))isint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have no experience with networkx, so I can't say either way, I just noticed that__getitem__ returnfloat (which is equivalent toint | float, so I was going by that. If you say that this will always beint, I'll believe you.
@srittau I tried something when |
Diff frommypy_primer, showing the effect of this PR on open source code: bokeh (https://github.com/bokeh/bokeh)+ src/bokeh/models/util/structure.py:165: note: ... from here: |
We'll need a |
Two small fixes to
networkx, let me know if it's better with separate PRs:Graph.subgraphcan also take a single node.degreetakes a single node, the return type is different so addoverloads.