Refactor Series.fillna() in a new style and add more tests #251
Conversation
kozlov-alexey
requested review from
PokhodenkoSA,
Hardcode84,
shssf,
AlexanderKalistratov and
densmirn
| if not isinstance(self, SeriesType): | ||
| raise TypingError('{} The object must be a pandas.series. Given: {}'.format(_func_name, self)) | ||
|
|
||
| if not (isinstance(axis, (types.Integer, types.UnicodeType, types.Omitted)) or axis is None): |
There was a problem hiding this comment.
Let's additionally check axis type on StringLiteral.
|
|
||
| if not (isinstance(inplace, types.Literal) and isinstance(inplace, types.Boolean) | ||
| or isinstance(inplace, types.Omitted) | ||
| or inplace == False): |
There was a problem hiding this comment.
More correct to compare booleans via operator is, inplace is False.
| if not ((method is None or isinstance(method, types.Omitted)) | ||
| and (limit is None or isinstance(limit, types.Omitted)) | ||
| and (downcast is None or isinstance(downcast, types.Omitted)) | ||
| ): |
There was a problem hiding this comment.
What do you think about splitting this condition on 3 independent ones? At the least it would look simpler.
There was a problem hiding this comment.
@densmirn I don't really think it's better, cause we'll duplicate raise statement, but I agree the check can be simplified if we, e.g. use a function like this:
def is_arg_omitted(arg, default_value):
has_default_value = (arg is default_value) if (default_value is None) else (arg == default_value)
return has_default_value or isinstance(arg, types.Omitted)
Can you maybe incorporate it into your TypeChecker in #241?
There was a problem hiding this comment.
We can update TypeChecker with the change, but with other commit (maybe with this one) and after TypeChecker pushed to master.
| # do operation inplace, fill the NA/NaNs in the same array and return None | ||
| if isinstance(self.dtype, types.UnicodeType): | ||
| # TODO: StringArrayType cannot resize inplace, and assigning a copy back to self._data is not possible now | ||
| return None |
There was a problem hiding this comment.
Maybe better to raise exception here something like unsupported...?
hpat/tests/test_series.py
Outdated
| def test_series_fillna_axis2(self): | ||
| '''Verifies Series.fillna() implementation handles 0 as axis argument''' | ||
| def test_impl(S): | ||
| print("Original series:\n", S) |
There was a problem hiding this comment.
Please remove debug print.
hpat/tests/test_series.py
Outdated
| axis_values = [0, 'index'] | ||
| for value in axis_values: |
There was a problem hiding this comment.
Why not merge the lines:
for axis in [0, 'index']:
|
@Hardcode84 @AlexanderKalistratov I'm going to fix remarks can you please review too? |
hpat/tests/test_series.py
Outdated
| @@ -1033,8 +1032,7 @@ def test_impl(S, axis): | |||
| hpat_func = hpat.jit(test_impl) | |||
|
|
|||
| S = pd.Series([1.0, 2.0, np.nan, 1.0, np.inf]) | |||
| axis_values = [0, 'index'] | |||
| for value in axis_values: | |||
| for value in [0, 'index']: | |||
There was a problem hiding this comment.
I would name the loop variable as axis.
There was a problem hiding this comment.
Yes, that's changed too.
There was a problem hiding this comment.
@shssf @Hardcode84 Can you please approve and merge if you have no more comments?
kozlov-alexey
force-pushed
the
feature/refactor_series_fillna
branch
from
abdd53b
to
f730ef5
Compare
| @@ -2512,47 +2513,121 @@ def hpat_pandas_series_median_impl(self, axis=None, skipna=True, level=None, num | |||
| return hpat_pandas_series_median_impl | |||
|
|
|||
|
|
|||
| @overload_method(SeriesType, 'dropna') | |||
There was a problem hiding this comment.
Sorry, I don't understand. You renamed series.dropna to series.fillna? What happend with series.dropna?
There was a problem hiding this comment.
That was an automatic merge issue, sorry, fixed it.
kozlov-alexey
force-pushed
the
feature/refactor_series_fillna
branch
from
f730ef5
to
100e7fc
Compare
kozlov-alexey
force-pushed
the
feature/refactor_series_fillna
branch
from
100e7fc
to
faffe61
Compare
|
|
||
| else: | ||
| def hpat_pandas_series_fillna_impl(self, value=None, method=None, axis=None, inplace=False, limit=None, downcast=None): | ||
| na_data_arr = hpat.hiframes.api.get_nan_mask(self._data) |
There was a problem hiding this comment.
I would vote for moving such function into some common place. These should be definitely not in api module.
But we could make it later of course.
shssf
added
FullyApproved
No description provided.