-
Notifications
You must be signed in to change notification settings - Fork 8
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
Feature/parents lookup #372
base: main
Are you sure you want to change the base?
Conversation
Hmm will take a look at the test errors in the morning and resolve |
@samuelduchesne I'm not sure why this test is failing
It passes fine on my machine and as far as I can tell I have not done anything that would affect that module. Everything else seems to be passing though. |
Okay, refactored this so that the PR is significantly simpler/less messy:
Comes with two tests:
The only test that is failing is the Really scratching my head on this one, as it passes fine on my machine when running it in isolation at least. Any ideas, @samuelduchesne ?
|
Will try to take a look tomorrow! |
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.
Is there a new function to traverse the hierarchy that is not parent_key_child_traversal
?
return cls._CREATED_OBJECTS[object_class.__name__ + "s"] | ||
|
||
@property | ||
def CREATED_OBJECTS(self): |
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 this be a private property instead of a public one? Trying to keep it clean as much as possible
return templates | ||
|
||
@classmethod | ||
def CREATED_OBJECTS(cls, object_class): |
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.
This has the same name as the property bellow?
@@ -437,6 +457,57 @@ def get_unique(self): | |||
|
|||
return obj | |||
|
|||
@property | |||
def Parents(self): | |||
""" Get the parents of an UmiBase Object""" |
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.
Stupid but can you remove the first space before each comment lol
Not for this pr - the ParentTemplates method illustrates climbing up the tree from a given leaf though. I don't think it's of utility in a top down approach. For me the idea for this PR is to just provide a simple and fairly minimal way to be able to determine ParentTemplates in the short term, so that we can then move to get The other aspect is that this can easily be upgraded if we build out the real-time edge tracking, since it essentially would just require updating the |
So this implements the determination of parents at runtime using the new
_CREATED_OBJECTS
functionality.The only thing that is a little off about it is that the wrapper objects don't have it implemented - e.g.
MassRatios
don't have parents (however theMaterial
inside of aMassRatio
will accurately find the Parents/ParentTemplates, same for aMaterial
inside of aLayer
finding the parent construction or aWeekSchedule
inside of aWeekSchedulePart
finding the parentYearSchedule
).Not a big deal IMO, since all UmiBase objects have it at least and it works properly.
There's a little bit of weirdness with the imports - rather than using
from x import y
we useimport x as y
in order to avoid circular imports. This is because we need to use the parent classes'_CREATED_OBJECTS
in the children, so the children must import the parents, but... the parents are also already importing the children. A way to avoid this would be to go back to storing the_CREATED_OBJECTS
inUmiBase
, but using a key for each parent class. That way all the children don't need to import the parent classes...