-
Notifications
You must be signed in to change notification settings - Fork 855
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
Implement map method for TimeSeries #163
Conversation
An implementation for map is added. It uses pandas' applymap method. A jupyter notebook to show the tests conducted. Resolves: #121
@Lovkush-A Thanks for your PR! I like your approach but have a couple of comments:
|
Unit tests test_map.py added. Included a small check, that the indices are in a valid range, in map method in TimeSeries class.
|
|
||
def map(self, | ||
fn: Callable[[float], float], | ||
cols: Optional[Union[List[int], int]] = None) -> 'TimeSeries': |
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.
@grll you'll probably have to change this as part of you refactoring
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.
yes we would need to replace int
indexing with str
indexing but nothing too serious here
Following hrzn's advice, various minor adjustments were made: * Correct various spacing/wording inconsistencies * Move contents of test_map.py into test_timeseries.py * Added sanity test case to tests for map * Changed type hints to np.number * Removed an unncessary call to self.pd_dataframe()
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.
Looks good to me.
Fixes #121
Summary
An implementation for map is added. It uses pandas' applymap method.
A jupyter notebook is included to show the tests conducted.
Other Information
I have added a couple of TODOs: