Skip to content

Conversation

adamnsch
Copy link
Collaborator

@adamnsch adamnsch commented Apr 11, 2025

Thank you for your contribution to the Graph Data Science Client project.

Before submitting this PR, please read Contributing to the Neo4j Ecosystem.

Make sure:

  • You signed the Neo4j CLA (Contributor License Agreement) so that we are allowed to ship your code in our library
  • Your contribution is covered by tests

@adamnsch adamnsch mentioned this pull request Apr 14, 2025
2 tasks
@adamnsch adamnsch force-pushed the entity_properties branch from c9ce0b5 to eac656b Compare April 16, 2025 09:39
Only `Node`, `Relationship` and `from_neo4j` updated so far.

Co-Authored-By: Florentin Dörre <florentin.dorre@neotechnology.com>
@adamnsch adamnsch force-pushed the entity_properties branch from eac656b to 9763371 Compare April 16, 2025 09:40
@adamnsch adamnsch force-pushed the entity_properties branch from f77c741 to f8b121c Compare April 17, 2025 07:34
@adamnsch adamnsch changed the title WIP: Move custom fields into properties dict Move non-NVL custom fields into a properties dict Apr 17, 2025
@adamnsch adamnsch marked this pull request as ready for review April 17, 2025 08:03


class PropertyType(Enum):
class ColorSpace(Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would prefer ValueSpace over ColorSpace.

Could at some doc string to clarify what we mean here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer ColorSpace as it also describes how one should interpret the colors provided to color_nodes. And a field/property can have float values, but you still might want to color it with a discrete space

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But yes, we should add doc strings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Collaborator

@FlorentinD FlorentinD left a comment

Choose a reason for hiding this comment

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

I like this change!

Its a good step allowing us other features we planned such as case-insensitive on data input :)


def from_dfs(
node_dfs: DFS_TYPE, rel_dfs: DFS_TYPE, node_radius_min_max: Optional[tuple[float, float]] = (3, 60)
def _from_dfs(
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the 2 methods now? the signature looks the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No they're not exactly the same. See the rename_properties arg, and how the from_gds loader uses it

Co-Authored-By: Florentin Dörre <florentin.dorre@neotechnology.com>
@adamnsch adamnsch merged commit 24e50b4 into main Apr 17, 2025
11 checks passed
@adamnsch adamnsch deleted the entity_properties branch April 17, 2025 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants