-
Notifications
You must be signed in to change notification settings - Fork 20
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
[WIP] Units system #160
base: main
Are you sure you want to change the base?
[WIP] Units system #160
Conversation
here is an example of what it would look like: |
@gonuke DO you have any though about this ? |
Is this still WIP or ready for review? (I can review things on request if they are still WIP, as well - of course) |
thx for offering. |
@gonuke, this is probably ready for a first set of review (test are not implemented yet...) the is the light version of the units management. Normalized metrics only depends on their direct parents (i.e. |
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.
The main part of this is hard to review because I have to relearn all of Cymetric's patterns. For now, here are some comments on some of the integration parts.
"""Checks if metric is already in the registry; adds it if not.""" | ||
normed_name = "norm_" + metric |
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.
Can we define a variable to be "norm_" to help guarantee consistency?
frames = [] | ||
for dep in m.dependencies: | ||
frame = self.eval(dep, conds=conds) | ||
frame = self.eval(dep, conds=conds, normed=False) |
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.
why do we force this to False
here? Maybe a comment?
@@ -40,7 +41,7 @@ def name(self): | |||
return self.__class__.__name__ | |||
|
|||
|
|||
def _genmetricclass(f, name, depends, scheme): | |||
def _genmetricclass(f, name, depends, scheme, register): |
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.
Update docstring to describe register
__doc__ = inspect.getdoc(f) | ||
|
||
def shema(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.
Seems like this method name is probably a typo?
And should this return self.schema
?
@@ -105,8 +110,9 @@ def dec(f): | |||
('Units', ts.STRING), | |||
('Mass', ts.DOUBLE) | |||
] | |||
_matregistry = { "Mass": ["Units", "kg"]} |
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 don't think I understand how the registry works?
return self._schema | ||
# fill in schema code | ||
registry = register | ||
#@property |
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.
delete these lines?
] | ||
resources = root_metric(name='Resources', schema=_resource_shema, registry=_resour_registry) | ||
|
||
#del _resour_registry, _resource_shema |
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.
remove commented lines
This work aims to allow unit management within Cyclus.