Wrapper for autoai_libs.nsfa transformer added #1329
Conversation
Signed-off-by: rafalll-maciasz <[email protected]>
There was a problem hiding this comment.
Overall this looks nice. I made a few suggestions for improvement.
Also, just to confirm: the input to fit and the input/output of transform can be arrays of arrays of anything? If, for example, the output is always an array of array of numbers, that should be specified (in the output_transform_schema in that case).
| class _NSFAImpl: | ||
| def __init__(self, **hyperparams): | ||
| self._wrapped_model = autoai_libs.cognito.transforms.transform_utils.NSFA( | ||
| **hyperparams | ||
| ) | ||
|
|
||
| def fit(self, X, **fit_params): | ||
| self._wrapped_model.fit(X, **fit_params) | ||
| return self | ||
|
|
||
| def transform(self, X): | ||
| result = self._wrapped_model.transform(X) | ||
| return result |
There was a problem hiding this comment.
It does not look like this impl class is needed.
Instead, simply pass autoai_libs.cognito.transforms.transform_utils.NSF to the lale.operators.make_operator call on line 125
| {"type": "array", "items": {"type": "number", "minimum": 0.0}}, | ||
| {"type": "array", "items": {"type": "integer", "minimum": 0}}, |
There was a problem hiding this comment.
I believe these are redundant. In JSON schema, integers are numbers. I would suggest removing the second line. If intent is to document that a common pattern is to provide specifically an array of integers, then it is reasonable to leave, although you may want a nested description string explaining.
Changes: