-
Notifications
You must be signed in to change notification settings - Fork 18
ZQS-721 Implement initial PoC of cost function logger #417
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #417 +/- ##
==========================================
+ Coverage 90.16% 90.33% +0.17%
==========================================
Files 59 60 +1
Lines 3354 3413 +59
Branches 553 556 +3
==========================================
+ Hits 3024 3083 +59
Misses 222 222
Partials 108 108
Continue to review full report at Codecov.
|
|
||
class _AttributePropagatingWrapper: | ||
def __init__(self, function, additional_attribute_source, attribute_overrides): | ||
object.__setattr__(self, "_function", function) |
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.
Its worth commenting (for all magic methods) in code why you need to modify them, so that future developers understand and don't break the functionality you are implementing.
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 added some documentation in the docstrings and several comments. I allowed myself to skip explaining why init and call are needed since those are rather obvious (but feel free to disagree). Let me know if the explanations provided now are sufficient.
|
||
@abc.abstractmethod | ||
def _act(self, result, params): | ||
pass |
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.
Shouldn't this raise a NotImplemtedError ?
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.
It doesn't have to, because you are not able to call this method unless you implement some concrete subclass of ConditionalSideEffect
. However, after adding docstring the pass statement is no longer needed too, so I removed it.
This PR implements a versatile framework for augmenting cost functions and includes a function logger implemented in this framework.
API description:
augment_cost_function
, which accepts a function to augment, and optional lists of augmentations for augmenting the function and its gradient. For instancefunc
hasnumber_of_layers
attribute thenaugmented_func
has it as well. Accessing such attributes (both querying and setting) are forwarded to the underlying function.augment_cost_function
already ensures that.Main architectural decisions:
ConditionalSideEffect
is implemented as an ABC for convenience. This specific type of augmentations passes through arguments to the wrapped function and propagates the return value, while triggering some side effect (e.g. logging) when the predicate is met. The predicates are the same assave_conditions
in our history recorders (which is why they are reused in the code proposed in this PR).FunctionLogger
is implemented as a subclass ofConditionalSideEffect
.ConditionalSideEffect
s need to be wrapped into a higher order function. In case ofFunctionLogger
it isfunction_logger
.bar
attribute then accessingbar
of the augmented function accesses augmentation's attribute (see tests for examples of this behavior)Some questions that are still open:
FunctionLogger
? How configurable should it be?common_augmentations
which would contain all augmentations that would be applied to both cost function and gradient prior to applying specific augmentations.