Skip to content
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

Merged
merged 17 commits into from
Jan 8, 2024
Merged

Conversation

tomvanmele
Copy link
Member

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

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.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

Sorry, something went wrong.

@tomvanmele
Copy link
Member Author

@Licini tree tests fail with this change. :class:compas.datastructures.Tree stores name and attributes in the data dict and probably overwrites the name implementation already present in compas.data.Data

@Licini
Copy link
Contributor

Licini commented Jan 4, 2024

I see, will make some modification on this branch in a bit

@Licini
Copy link
Contributor

Licini commented Jan 4, 2024

@tomvanmele updated the tree, now the tests pass

src/compas/data/data.py Outdated Show resolved Hide resolved
}

def __init__(self, name=None, attributes=None):
super(TreeNode, self).__init__(name=name)
self.attributes = attributes or {}
if attributes:
Copy link
Member Author

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
    ...

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Member Author

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

  1. keep the attributes dict in the data dict and convert the node explicitly to/from data
  2. 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?

Copy link
Contributor

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?

Copy link
Contributor

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...

Copy link
Member Author

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?

Copy link
Member Author

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

Copy link
Contributor

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

@tomvanmele tomvanmele marked this pull request as ready for review January 4, 2024 21:58
Copy link
Contributor

@Licini Licini left a 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!

@Licini Licini mentioned this pull request Jan 5, 2024
@Licini
Copy link
Contributor

Licini commented Jan 5, 2024

Another thing, would it be possible to have something like to_dict, outputting same content of to_jsonstring but as dict instead of string? Again, doesn't have to be in this PR

@tomvanmele tomvanmele merged commit eb45611 into main Jan 8, 2024
18 checks passed
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.

None yet

2 participants