Implement Series.head() #223

1e-to · 2019-10-14T11:38:08Z

Some tests skips, until problem with index fix

akharche · 2019-10-14T11:56:13Z

hpat/datatypes/hpat_pandas_series_functions.py

+    """
+    Pandas Series method :meth:`pandas.Series.head` implementation.
+    .. only:: developer
+       Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head1


There are more tests

akharche · 2019-10-14T11:57:28Z

hpat/datatypes/hpat_pandas_series_functions.py

+    if not isinstance(self.index, types.NoneType):
+        def hpat_pandas_series_head_impl(self, n=5):
+
+            return pandas.Series(self._data[:n], self._index[:n])


What if index is None? I guess it does not work with the current index implementation

Add raise exception to index None?

Current implementation allows to call head() even if index is set to None

Add raise exception to index None?

No, just create pandas.Series with no index

akharche · 2019-10-14T11:59:03Z

hpat/hiframes/hiframes_typed.py

+        #     name = self._get_series_name(series_var, nodes)
+        #
+        #     return self._replace_func(
+        #         func, (data, index, n_arg, name), pre_nodes=nodes)


Dataframe.head() is based on it. Does it work with new style?

We have not tests for Dataframe.head() (only one skiped)
Should I write it and test?

test_df_head1 is skipped due to 'dtype' fail. I guess it has been fixed yet, let's check. Nevertheless, test was skipped on Windows only

densmirn · 2019-10-14T11:56:30Z

hpat/datatypes/hpat_pandas_series_functions.py

+        raise TypingError(
+            '{} The parameter must be an integer type. Given type n: {}'.format(_func_name, n))
+
+    if not isinstance(self.index, types.NoneType):


In case if index is none hpat_pandas_series_head returns None. Maybe better temporarily (before fixing indexing) to raise exception in this case?

in case on index=None the function returns hpat_pandas_series_head_index_impl

Now it's so, but before it wasn't so: ad8a03b.

densmirn · 2019-10-14T12:00:47Z

hpat/hiframes/series_kernels.py

-    'head': lambda A, I, k, name: hpat.hiframes.api.init_series(A[:k], None, name),
+    # 'head': lambda A, I, k, name: hpat.hiframes.api.init_series(A[:k], None, name),


Is the change exactly needed? I think that affects nothing because you commented out head in old style.

Yes you right, I recommented it

@1e-to In this case tests on parallelism check will be broken. Please refer to any latest PRs, like PR #186

@shssf Should I delete existing tests on parallelism and write them in another style, or comment old?

@1e-to No, I don't think you need it

shssf · 2019-10-14T14:33:54Z

hpat/datatypes/hpat_pandas_series_functions.py

+
+    if not isinstance(self, SeriesType):
+        raise TypingError(
+            '{} The object must be a pandas.series. Given self: {}'.format(_func_name, self))


no need Given self: here. Replace it with Given:

shssf · 2019-10-14T16:35:25Z

Need to redesign tests a bit
enable all tests
fix boxing/unboxing

Some tests skips, until problem with index fix

densmirn · 2019-10-25T09:49:35Z

hpat/datatypes/hpat_pandas_series_functions.py

+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head1
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_default1
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_index1
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_index2
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_index3
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_index4
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_parallel1
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_index_parallel1
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_index_parallel2
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_noidx
+#        Test: python -m hpat.runtests hpat.tests.test_series.TestSeries.test_series_head_idx


Look like too many tests listed in docstring. Maybe better to represent the list as the pattern with parameter -k: python -m hpat.runtests -k hpat.tests.test_series.TestSeries.test_series_head*

densmirn · 2019-10-25T09:50:15Z

hpat/datatypes/hpat_pandas_series_functions.py

+#     Returns
+#     -------
+#     :obj:`pandas.Series`
+#          returns The first n rows of the caller object.


The -> the

densmirn · 2019-10-25T09:51:44Z

hpat/datatypes/hpat_pandas_series_functions.py

+#     n: :obj:`int`
+#                input argument, default 5


Let's move default 5 after type of the parameter:

n: :obj:`int`, default 5

densmirn · 2019-10-25T09:52:29Z

hpat/datatypes/hpat_pandas_series_functions.py

+#     _func_name = 'Method head().'
+#
+#     if not isinstance(self, SeriesType):
+#         raise TypingError('{} The object must be a pandas.series. Given: {}'.format(_func_name, self))
+#
+#     if not isinstance(n, (types.Integer, types.Omitted)) and n != 5:
+#         raise TypingError('{} The parameter must be an integer type. Given type n: {}'.format(_func_name, n))
+#
+#     if isinstance(self.index, types.NoneType):
+#         def hpat_pandas_series_head_impl(self, n=5):
+#             return pandas.Series(self._data[:n])
+#
+#         return hpat_pandas_series_head_impl
+#     else:
+#         def hpat_pandas_series_head_index_impl(self, n=5):
+#             return pandas.Series(self._data[:n], self._index[:n])
+#
+#         return hpat_pandas_series_head_index_impl


Why is the implementation commented out?

densmirn · 2019-10-25T09:54:04Z

hpat/tests/test_series.py

+
+        hpat_func = hpat.jit(test_impl)
+
+        data_test = [[6, 6, 2, 1, 3, 3, 2, 1, 2],


I propose to use globally defined data as input data in tests: f2435b0#diff-deca39d332649cea819383154a5d2cb3R39-R62

densmirn · 2019-10-25T09:54:50Z

hpat/tests/test_series.py

+
+        hpat_func = hpat.jit(test_impl)
+
+        data_test = [[6, 6, 2, 1, 3, 3, 2, 1, 2],


The same, could you use globally defined input data? If it's required the global data could be changed.

densmirn · 2019-10-25T09:56:01Z

hpat/tests/test_series.py

+
+            hpat_func_param1 = hpat.jit(test_impl_param)
+
+            for param1 in [0, 3, 10]:


Let's rename param1 to n, because n is real name of parameter of the method.

densmirn · 2019-10-25T09:56:58Z

hpat/tests/test_series.py

+            hpat_func_param1 = hpat.jit(test_impl_param)
+
+            for param1 in [0, 3, 10]:
+                result_param1_ref = test_impl_param(S, param1)


I think it's enough to name such variable as ref_result or jit_result.

densmirn · 2019-10-25T09:57:54Z

hpat/tests/test_series.py

+                for param1 in [1, 3, 7]:
+                    result_param1_ref = test_impl_param(S, param1)
+                    result_param1 = hpat_func_param1(S, param1)


The same:
param1 -> n
result_param1_ref -> ref_result
result_param1 -> jit_result

densmirn · 2019-10-25T10:02:37Z

hpat/tests/test_series.py

+                result_ref = test_impl(S)
+                result = hpat_func(S)


Let's rename the variables:
result_ref -> ref_result
result -> jit_result

densmirn · 2019-10-25T10:05:51Z

hpat/tests/test_series.py

+
+    @unittest.skip("Broke another three tests")
+    def test_series_head_idx(self):
+        def test_impl(S):


Don't we want to add implementation where series is constructed? I mean to test functionality without unboxing series.

densmirn · 2019-10-25T14:17:50Z

hpat/datatypes/hpat_pandas_series_functions.py

+        return hpat_pandas_series_head_impl
+    else:
+        def hpat_pandas_series_head_index_impl(self, n=5):
+            return pandas.Series(self._data[:n], self._index[:n])


Could you construct output Series passing parameter name as self._name? It should be supported.

densmirn · 2019-10-25T14:20:31Z

hpat/tests/test_series.py

            S = pd.Series(input_data)
+            for n in [1, 3, 2]:


Please add negative and zero n in the testing.

densmirn · 2019-10-25T14:21:49Z

hpat/tests/test_series.py

+                result_jit = hpat_func(S, n)
+                pd.testing.assert_series_equal(result_jit, result_ref)
+
+    @unittest.skip("Not pass")


Could you add more clear description why the test should be skipped?

densmirn · 2019-10-25T14:22:15Z

hpat/tests/test_series.py

+        hpat_func = hpat.jit(test_impl)
+        for input_data in test_global_input_data_integer64:
+            S = pd.Series(input_data)
+            for n in [2, 3]:


Please add negative and zero n here and in other tests.

densmirn · 2019-10-25T14:22:40Z

hpat/tests/test_series.py

+                result_jit = hpat_func(S, n)
+                pd.testing.assert_series_equal(result_jit, result_ref)
+
+    @unittest.skip("Not pass")


Needed more clear description.

densmirn · 2019-10-25T14:24:50Z

hpat/datatypes/hpat_pandas_series_functions.py

+    Pandas Series method :meth:`pandas.Series.head` implementation.
+
+    .. only:: developer
+       Test: python -m -k hpat.runtests hpat.tests.test_series.TestSeries.test_series_head*


python -m hpat.runtests -k hpat.tests.test_series.TestSeries.test_series_head*

1e-to requested review from fschlimb, shssf, kozlov-alexey and densmirn October 14, 2019 11:38

akharche suggested changes Oct 14, 2019

View reviewed changes

densmirn reviewed Oct 14, 2019

View reviewed changes

shssf reviewed Oct 14, 2019

View reviewed changes

1e-to requested a review from akharche October 17, 2019 09:48

etotmeni and others added 8 commits October 23, 2019 16:25

Implement Series.head()

ba4ac7c

Some tests skips, until problem with index fix

Add tests to docs

0acac70

PR 223. Fix method algo

d214957

PR 223. typo fixed

0619168

PR 223. typo1 fixed

3fb5d96

Add test for all

7b99c0a

Refactor tests for index

5ac5adb

WIP

09f857f

1e-to force-pushed the implement_head branch from fbc8ebc to 09f857f Compare October 24, 2019 08:17

Fix pass tests

4eaae30

densmirn suggested changes Oct 25, 2019

View reviewed changes

densmirn reviewed Oct 25, 2019

View reviewed changes

densmirn added the Waiting on author label Oct 25, 2019

Fix docs and tests

cca270a

densmirn suggested changes Oct 25, 2019

View reviewed changes

densmirn reviewed Oct 25, 2019

View reviewed changes

Small fixes

617d972

1e-to added Ready for Review and removed Waiting on author labels Oct 25, 2019

Merge branch 'master' into implement_head

66a1971

shssf approved these changes Oct 25, 2019

View reviewed changes

shssf merged commit 619bacf into IntelPython:master Oct 25, 2019

		'head': lambda A, I, k, name: hpat.hiframes.api.init_series(A[:k], None, name),
		# 'head': lambda A, I, k, name: hpat.hiframes.api.init_series(A[:k], None, name),


		hpat_func = hpat.jit(test_impl)

		data_test = [[6, 6, 2, 1, 3, 3, 2, 1, 2],


		hpat_func_param1 = hpat.jit(test_impl_param)

		for param1 in [0, 3, 10]:

Implement Series.head() #223

Implement Series.head() #223

Conversation

1e-to commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shssf commented Oct 14, 2019

densmirn Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

densmirn Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

densmirn Oct 25, 2019 •

edited

Loading

densmirn Oct 25, 2019 •

edited

Loading