Added drop operator #313
Added drop operator #313
Conversation
Coverage reportMain: 91.13% | PR: 91.14% | Diff: 0.01 ✅ |
temporian/core/operators/drop.py
Outdated
| @typecheck | ||
| @compile | ||
| def drop( | ||
| input: EventSetOrNode, feature_names: str | List[str] |
There was a problem hiding this comment.
The | syntax for type union was only introduced in 3.10, and temporian supports 3.8 and up (which is why the tests GH action is failing on py3.8) - we need to use Union
temporian/core/operators/drop.py
Outdated
| raise TypeError( | ||
| "Features" | ||
| f" {[fn for fn in feature_names if fn not in input_features]} are" | ||
| " not present in the input features" |
There was a problem hiding this comment.
nit: not present in the input.
temporian/core/operators/BUILD
Outdated
| deps = [ | ||
| ":base", | ||
| "//temporian/core:operator_lib", | ||
| "//temporian/core/data:node", | ||
| "//temporian/core/data:schema", | ||
| "//temporian/proto:core_py_proto", | ||
| ], |
There was a problem hiding this comment.
these two don't seem necessary:
"//temporian/core/data:schema",
"//temporian/proto:core_py_proto",
and these seem missing:
"//temporian/core:compilation",
"//temporian/core:typing",
"//temporian/core/operators:select",
try running the python tools/build_cleaner.py (though its weird these tests passed with missing deps)
temporian/core/operators/drop.py
Outdated
| from typing import List | ||
| from temporian.core import operator_lib | ||
| from temporian.core.compilation import compile | ||
| from temporian.core.data.node import ( | ||
| EventSetNode, | ||
| create_node_new_features_new_sampling, | ||
| ) | ||
| from temporian.core.operators.base import Operator | ||
| from temporian.core.typing import EventSetOrNode | ||
| from temporian.proto import core_pb2 as pb | ||
| from temporian.utils.typecheck import typecheck | ||
| from temporian.core.operators.select import select |
There was a problem hiding this comment.
several unused imports - please install pylint and address the unused import warnings (maybe add it to the workspace's recommended extensions too while you're at it)
temporian/core/operators/test/BUILD
Outdated
| deps = [ | ||
| "//temporian/implementation/numpy/data:io", | ||
| "//temporian/test:utils", | ||
| "//temporian/core/operators:drop", |
There was a problem hiding this comment.
unneeded (you're calling drop from the EventSet, not importing it from its module)
javiber
force-pushed
the
drop-operator
branch
from
fe5b736
to
4be6cda
Compare
Coverage reportMain: 91.10% | PR: 91.12% | Diff: 0.01 ✅ |
Also added pylint to recommended extesions
javiber
force-pushed
the
drop-operator
branch
from
4be6cda
to
9494348
Compare
Coverage reportMain: 91.10% | PR: 91.11% | Diff: 0.01 ✅ |
Coverage reportMain: 91.10% | PR: 91.10% | Diff: 0.00 ✅ |
Coverage reportMain: 91.10% | PR: 91.10% | Diff: 0.00 ✅ |
Behind the scenes, it just uses the select implementation.
I am not sure about the dependencies in the build files, it's working but it was the result of many trial-and-error