-
Notifications
You must be signed in to change notification settings - Fork 306
Updated SimpleFilterTask and wrote additional unit tests #438
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
Conversation
zigaLuksic
left a comment
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 am not sure i understand the 'returns a shallow copy'
what exactly is shallowly copied here?
We return a new instance of an EOPatch and by default all features are filtered and moved into it. So it's basically a shallow copy. |
Not entirely sure I would call it a copy, it's more of a view (since we are returning views of numpy features), but I see your reasoning now. |
|
|
||
|
|
||
| class SimpleFilterTask(EOTask): | ||
| class SimpleFilterTask(EOTask, Generic[_T]): |
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.
wouldnt it be possible to omit all the generic stuff by using
FilterFuncType = Union[Callable[[np.ndarray], bool], Callable[[dt.datetime], bool]]While i'm a big fan of using advanced type theory, I'm worried that this migh class with future improvements to EOTasks since they would need to be generics (and i'm not sure how generics clash in python).
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.
Ah, too bad. Ok, I removed Generic and TypeVar and used the normal union. Then I had to set a type Iterable to the parameter in _get_filtered_indices method and mypy is also fine with that.
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.
well to be entirely precise this would also depend on what the user inputs as the feature right? So technically overload would work best here. But perhaps let's not go there :)
Codecov Report
@@ Coverage Diff @@
## develop #438 +/- ##
===========================================
+ Coverage 82.71% 82.72% +0.01%
===========================================
Files 59 59
Lines 5530 5535 +5
===========================================
+ Hits 4574 4579 +5
Misses 956 956
Continue to review full report at Codecov.
|
There are a few minor changes in the functionality:
_update_other_datamethod because it was useless. Instead, any such update can be performed in a separate task.Note: I'm not sure if I correctly handled type of
filter_funcbut it is a bit tricky because it depends on feature type offeatureparameter.