-
Notifications
You must be signed in to change notification settings - Fork 9
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
Mutable tree #45
Mutable tree #45
Conversation
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 1087 1106 +19
=========================================
+ Hits 1087 1106 +19
Continue to review full report at Codecov.
|
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.
Looks good! Some minor comments above about docstrings, commenting and removing commented-out code from test cases 👍
if isinstance(subtree, dict): | ||
subtree = subtree[level] | ||
else: | ||
subtree = subtree[int(level)] |
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.
Could you just explain this part a little to me? You traverse down the tree and assume if the subtree isn't a dict then the next path level is always going to be an integer, is my understanding right?
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.
Pretty much, yeah. This was based off the traversal stuff in the set method, which does the same thing. The idea being that, if you're still traversing the path you set, you're either:
at a branch node of the tree, thus the next part should be another dict
at a node that contains a list, thus the next part of the path should be the list index
Then, obviously, if its neither of these things, you're path is invalid and it raises the ParameterTreeError
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 think some comments dotted throughout the code explaining these details at each step would be useful
…ble tree. Does not work the other way around, this may have to remain as a limitation
…ed tests and modified the recursive merge method to now allow this
Created a Mutable Flag for Parameter Tree. Setting this flag allows the creation and deletion of nodes in the parameter tree. This includes branch nodes and leaf nodes. Care should be taken when using a Mutable Parameter Tree to avoid overwriting Accessors and other nodes accidentally.
This change also removed the (long depreciated) Callback system from Parameter Tree