Series.max() in new style #189
Conversation
akharche
requested review from
1e-to,
densmirn,
fschlimb,
kozlov-alexey and
shssf
|
|
||
| def hpat_pandas_series_max_impl(self, axis=0, skipna=True, level=None, numeric_only=None): | ||
| if skipna: | ||
| return np.nanmax(self._data) |
There was a problem hiding this comment.
Do we expect Numpy data type returned? I suspect we need Series data type constructed here
There was a problem hiding this comment.
We return scalars because Series can be returned if level specified. But we don't have multi indexing now and parameter 'level' is not supported.
| dtype = (pandas_timestamp_type | ||
| if isinstance(dtype, types.NPDatetime) else dtype) | ||
| return signature(dtype, *args) | ||
| # @bound_function("series.max") |
There was a problem hiding this comment.
This is good. This is typing stage. What happen if comment out definition of the function here https://github.com/IntelPython/hpat/blob/ea6b1a22ba3a04d4495b6a69bb762c09dc41be67/hpat/hiframes/series_kernels.py#L499
|
|
||
|
|
||
| @overload_method(SeriesType, 'max') | ||
| def hpat_pandas_series_max(self, axis=0, skipna=True, level=None, numeric_only=None): |
There was a problem hiding this comment.
Parameters are diffrent https://pandas.pydata.org/pandas-docs/version/0.25/reference/api/pandas.Series.max.html?highlight=max#pandas.Series.max
Please fix the parameters
There was a problem hiding this comment.
parameter skipna=True is still diffrent
There was a problem hiding this comment.
Sorry, looked into pandas sources and found
def max(self, axis=None, skipna=True, *args, **kwargs):
it looks like they have documentation issue
|
|
||
| import operator | ||
| import pandas | ||
| import numpy as np |
There was a problem hiding this comment.
Could you please do not rename numpy?
There was a problem hiding this comment.
why not? This is how numpy is used is everywhere else.
There was a problem hiding this comment.
@fschlimb to keep clear in the code what exactly is used. This is not a strong requirement.
There was a problem hiding this comment.
I understand and I also generally prefer fully qualified names. In this particular case we should stay with common practice (e.g. import numpy as np and import pandas ad pd)
akharche
force-pushed
the
impl_series_max
branch
2 times, most recently
from
83937ec to
7a25ddd
Compare
…l_series_max # Conflicts: # hpat/datatypes/hpat_pandas_series_functions.py
3 participants
Due to overloaded method 'max' in numba it calls array.max for SeriesType instead of hpat_pandas_series_max. Firstly, need fix in importing.