Support relationship type as str in gds.graph.relationshipProperties.stream
#658
Conversation
✅ Deploy Preview for neo4j-graph-data-science-client canceled.
|
orazve
changed the title
relationshipType set as str, not as list in gds.graph.relationshipPropert[y|ies].stream
orazve
changed the title
relationshipType set as str, not as list in gds.graph.relationshipPropert[y|ies].streamrelationshipType as str, not as list only in gds.graph.relationshipPropert[y|ies].stream
orazve
changed the title
relationshipType as str, not as list only in gds.graph.relationshipPropert[y|ies].streamgds.graph.relationshipProperties.stream
Call properly the rel_id in kge notebook
orazve
force-pushed
the
rel-prop-error
branch
2 times, most recently
from
6dae722 to
b64bdae
Compare
| ) | ||
|
|
||
|
|
||
| class GraphElementPropertyRunner(GraphEntityOpsBaseRunner): |
There was a problem hiding this comment.
The def relationshipProperty is the only one what uses this runner..? 😮
There was a problem hiding this comment.
Yes, so it was renamed, moved closer to GraphRelationshipPropertiesRunner and added wrapping into a list if string was passed.
|
and changelog entries, I think it's two separate bug fixes? |
FlorentinD
left a comment
FlorentinD
left a comment
There was a problem hiding this comment.
Great find!
I have a small suggestion to transform the parameter later.
| self, G: Graph, relationship_property: str, relationship_types: Strings = ["*"], **config: Any | ||
| ) -> DataFrame: | ||
| self._namespace += ".stream" | ||
| relationship_types = [relationship_types] if isinstance(relationship_types, str) else relationship_types |
There was a problem hiding this comment.
i would suggest to do this transformation as part of _handle_properties.
In our signature, we have Strings here, but these procedures do not always handle both str and list of str.
Such as https://neo4j.com/docs/graph-data-science/current/management-ops/graph-reads/graph-stream-relationships/#_syntax.
For nodeLabels we do support both str and list of str
There was a problem hiding this comment.
created a card to fix this inconsistency as well in gds
There was a problem hiding this comment.
Thanks, I've made an experiment where I put this wrapping into a list in _handle_properties and it breaks nodeProperty.stream and relationshipProperty.stream cases. And I think we won't need this wrapping when we align APIs after getting this card done.
So I prefer to leave it as it is now and merge.
3 participants
The problem from issue #554 was reproduced locally and showed that
gds.graph.relationshipProperties.streamhas different API in Cypher and Python Client sides: Cypher can getlist[str]asrelationshipType, but Python Client can getstrandlist[str].This PR contain:
gds.graph.relationshipProperty.streamwithout callingseparate_property_columns=Trueparametergds.graph.relationshipProperties.streamwith relationshipType asstrQuestion: why this problem is not reproduced by CI, where we run
kge-predict-transe-pyg-train.ipynbwhich callsgds.graph.relationshipProperties.streamfunction with relationshipType asstr, not withlist[str]?Before submitting this PR, please read Contributing to the Neo4j Ecosystem.
Make sure: