-
Notifications
You must be signed in to change notification settings - Fork 17
Feature/#336 - Filter numpy array exposed type #768
Conversation
☂️ Python Cov
Overall Coverage
New FilesNo new covered files... Modified Files
|
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.
lgtm
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.
-
excel.py should also be updated with the return type of method
_read_as_numpy
. -
I would like to challenge the design:
- I am not sure to understand the purpose of the
_FilterDataNode
class - I would like to move the whole filtering logic out of the
DataNode
class. Thedatanode.filter
method could delegate the filtering logic to a dedicated class likeFilter
. What do you think?
- I am not sure to understand the purpose of the
2.1. Me neither 🥲 I think we can remove it since it is not documented at all. 2.2. I agree with moving the logic to a dedicate file. And if I understand correctly, the filter() method should still return the filtered data directly, not returning the Filter class right? |
fb09f79
to
d83b52d
Compare
In this PR: New feature:
Refactoring:
Bug fixes:
|
#336
A few things we need to consider: