-
Notifications
You must be signed in to change notification settings - Fork 8
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
Allow custom Node_Info subclasses #26
Conversation
nxontology/ontology.py
Outdated
def __init__( | ||
self, | ||
graph: nx.DiGraph | None = None, | ||
node_info_class: type[Node_Info[Node]] = Node_Info, |
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.
The NXOntology
class has methods that return (or use internally) instances of the Node_Info
class. I'd like the user to be able to be able to provide a Node_Info
subclass with additional functionality. The approach in 756eb51 seems to work except for typing (see test_custom_node_info_class
).
Some thoughts:
- it could get complicated if typing becomes like
nxo: NXOntology[str, NodeInfoSubclass]
- we might also want to enable custom
Similarity
andSimilarityIC
subclasses
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.
If I understand this correctly, you can't do this "properly" because higher-kinded types are not supported in mypy/python python/typing#548 . You could do:
class NXOntology(Freezable, Generic[Node, NodeInfoSubclass]):
...
but that's not really what you want, apart from being needlessly complicated, further allows for Node
not being in sync with NodeInfoSubclass
.
Another alternative would be to have NXOntology
work on NodeInfoSubclass
or other box class only, but that doesn't seem like it's worth the work.
You may need to ask yourself what is more useful from UX perspective:
class NXOntology(Freezable, Generic[Node]):
...
or
class NXOntology(Freezable, Generic[NodeInfoSubclass]):
...
I suspect the latter might be more "useful", but I'm not familiar with nxontology enough to really know.
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.
You could also allow and document how to provide your own subclasses of NXOntology
, and probably supply an abstract/base version of NXOntology
+ vanilla implementation. Here's PoC:
from abc import abstractmethod
from collections.abc import Hashable
from typing import Generic, TypeVar
Node_T = TypeVar("Node_T", bound=Hashable)
NodeInfo_T = TypeVar("NodeInfo_T")
class NodeInfo(Generic[Node_T]):
...
class CustomNodeInfo(NodeInfo[Node_T]):
def prop(self) -> Node_T: # type: ignore[empty-body]
...
class BaseFoo(Generic[Node_T, NodeInfo_T]):
def __init__(self) -> None:
...
@classmethod
@abstractmethod
def get_node_info_cls(cls) -> type[NodeInfo_T]:
...
def fn(self) -> NodeInfo_T: # type: ignore[empty-body]
...
def fn_2(self) -> Node_T: # type: ignore[empty-body]
...
class StandardFoo(BaseFoo[Node_T, NodeInfo[Node_T]]):
@classmethod
def get_node_info_cls(cls) -> type[NodeInfo[Node_T]]:
return NodeInfo
class CustomFoo(BaseFoo[Node_T, CustomNodeInfo[Node_T]]):
@classmethod
def get_node_info_cls(cls) -> type[CustomNodeInfo[Node_T]]:
return CustomNodeInfo
if __name__ == "__main__":
std = StandardFoo[str]()
reveal_type(std)
reveal_type(std.fn())
reveal_type(std.fn_2())
custom = CustomFoo[float]()
reveal_type(custom)
reveal_type(custom.fn())
reveal_type(custom.fn().prop())
reveal_type(custom.fn_2())
If you run mypy on that code you will see:
pipelines/foo.py:48: note: Revealed type is "pipelines.foo.StandardFoo[builtins.str]"
pipelines/foo.py:49: note: Revealed type is "pipelines.foo.NodeInfo[builtins.str]"
pipelines/foo.py:50: note: Revealed type is "builtins.str"
pipelines/foo.py:53: note: Revealed type is "pipelines.foo.CustomFoo[builtins.float]"
pipelines/foo.py:54: note: Revealed type is "pipelines.foo.CustomNodeInfo[builtins.float]"
pipelines/foo.py:55: note: Revealed type is "builtins.float"
pipelines/foo.py:56: note: Revealed type is "builtins.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.
I've implemented a partial solution that does not define a base NXOntology class (with a generic node type and a generic node info type). The partial solution is in c503f1c and is used by related-sciences/nxontology-ml@208bee7.
From test_custom_node_info_class
, the partial solution allows setting a custom node info class via _get_node_info_cls
:
nxontology/nxontology/tests/ontology_test.py
Lines 159 to 172 in c503f1c
class CustomNodeInfo(Node_Info[str]): | |
@property | |
def custom_property(self) -> str: | |
return "custom" | |
class CustomNxontology(NXOntology[str]): | |
@classmethod | |
def _get_node_info_cls(cls) -> Type[CustomNodeInfo]: | |
return CustomNodeInfo | |
def node_info(self, node: str) -> CustomNodeInfo: | |
info = super().node_info(node) | |
assert isinstance(info, CustomNodeInfo) | |
return info |
Note how the subclass also has to override node_info
to specify the return type as CustomNodeInfo
. This could get annoying if there become many methods that return CustomNodeInfo
. @ravwojdyla I believe your solution would fix this, but I hit some mypy errors implementing. Perhaps I'll give it a second go.
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've merged this PR and opened #27 with the introduction of NodeInfoT
typing, although there's a large amount of mypy errors.
No description provided.