-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add storage and serialisation of general attributes #1247
Conversation
@Licini tree tests fail with this change. :class: |
I see, will make some modification on this branch in a bit |
@tomvanmele updated the tree, now the tests pass |
} | ||
|
||
def __init__(self, name=None, attributes=None): | ||
super(TreeNode, self).__init__(name=name) | ||
self.attributes = attributes or {} | ||
if attributes: |
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.
should it not be
def __init__(self, **kwargs):
super(TreeNode, self).__init__(**kwargs)
self._parent = None
...
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.
and the same for the tree itself?
and should we not also remove the attributes from the data dict?
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.
should it not be
def __init__(self, **kwargs): super(TreeNode, self).__init__(**kwargs) self._parent = None ...
right, indeed! updated now
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.
and the same for the tree itself? and should we not also remove the attributes from the data dict?
You mean like this?
@property
def data(self):
return {
# "attributes": self.attributes,
"children": [child.data for child in self.children],
}
We would lost track of name then, unless we put it back explicitly.
At least for TreeNode isn't it nice to keep the whole attributes dict so there is a bit more flexibility?
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.
for the tree node there are two options
- keep the attributes dict in the data dict and convert the node explicitly to/from data
- don't include the attributes dict in the data dict and let the encoder/decoder handle to/from (then the attributes are included implicitly)
the file size of 1. is much smaller and is similar to what happens to nodes in a graph and vertices in a mesh. however, there the behaviour is not so explicit since the nodes and vertices are not separate objects.
if a tree node is not meant to exist on its own (like graph nodes and mesh vertices), perhaps we should make sure that they cannot be converted to/from data independently?
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'm more in favor of option.1. imo, the node can be create independently but there is no need to serialise them outside context of tree. They also don't need guid, because their structural location in the tree makes them naturally unique. Maybe it should even not inherant from Data
at all... Well this is distracting the purpose of this PR, shall we keep it for now and open another PR for it?
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.
mmm.... actually... option 2 is also not bad........ well let's have this discussion in another PR...
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.
currently everything works, so we can merge this and continue the conversation later. but we need to make sure the behaviour is consistent everywhere and 100% transparent.
@gonzalocasas @chenkasirer opinions?
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.
mmm.... actually... option 2 is also not bad........ well let's have this discussion in another PR...
i also prefer option 1 btw
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.
currently everything works, so we can merge this and continue the conversation later. but we need to make sure the behaviour is consistent everywhere and 100% transparent.
@gonzalocasas @chenkasirer opinions?
yes agree
…into data-interface
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.
making the tree thing a Issue, everything else lgtm!
Another thing, would it be possible to have something like |
What type of change is this?
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.md
file in theUnreleased
section under the most fitting heading (e.g.Added
,Changed
,Removed
).invoke test
).invoke lint
).compas.datastructures.Mesh
.