Skip to content
This repository was archived by the owner on Feb 2, 2024. It is now read-only.

Conversation

@PokhodenkoSA
Copy link
Contributor

Implemented Series.sum() via numpy.nansum().
Is it correct solution or should I implement it in another way?

Copy link
Contributor

@shssf shssf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a tests for this functionality? Did you try them with HPAT_CONFIG_PIPELINE_HPAT=0 to make sure Numba could work with it?
Overall approach is good.

@PokhodenkoSA
Copy link
Contributor Author

With HPAT_CONFIG_PIPELINE_HPAT=0 the old test hpat.tests.test_series.TestSeries.test_series_sum2 do not work:

No definition for lowering <built-in function add>(series(float64, array(float64, 1d, C), none, False), series(float64, array(float64, 1d, C), none, False)) -> series(float64, array(float64, 1d, C), none, False)

File "hpat\tests\test_series.py", line 859:
        def test_impl(S):
            return (S + S).sum()
            ^

[1] During: lowering "$0.3 = S + S" at c:\Users\spokhode\Documents\hpat-workspace\hpat\hpat\tests\test_series.py (859)

As I can understand add operator for Series is not implemented yet. @shssf should I fix it in this PR or in new one?

@shssf
Copy link
Contributor

shssf commented Oct 10, 2019

@PokhodenkoSA I see the test (return (S + S).sum()) is testing two diffrent things: function sum() and operator+
This test should be disabled until we implement all required functionality. Example in PR #189 (@unittest.skipIf(hpat.config.config_pipeline_hpat_default can be used)
Please create new tests for particular sum()

Operators should be implemented separately (in other PRs).

@shssf shssf removed the question label Oct 10, 2019
@shssf
Copy link
Contributor

shssf commented Oct 12, 2019

please resolve conflicts

@PokhodenkoSA
Copy link
Contributor Author

PokhodenkoSA commented Oct 17, 2019

@shssf, I need help.

I have made all modifications we usually do for disabling old approach for sum:

  1. Implement @overload_method in hpat/datatypes/hpat_pandas_series_functions.py
  2. Removed sum from list in _run_call_series() in hpat/hiframes/hiframes_typed.py
  3. Disable adding resolve_sum to SeriesAttribute in hpat/hiframes/pd_series_ext.py
  4. Commented/uncommented sum in hpat/hiframes/series_kernels.py:
series_replace_funcs = {
    # 'sum': _column_sum_impl_basic,

Moreover, I have fixed call for hpat.hiframes.pd_series_ext.SeriesAttribute.resolve_sum() in hpat/hiframes/pd_dataframe_ext.py (SeriesAttribute has no resolve_sum, see point 3 above).

Finally I got this errors:

ERROR: test_str_contains_noregex (hpat.tests.test_hiframes.TestHiFrames)
  File "...\AppData\Local\Continuum\anaconda3\envs\hpat-sum-env\lib\site-packages\numba\datamodel\models.py", line 696, in getter
    raise TypeError("expecting {0} but got {1}".format(*args))
TypeError: expecting {i8*, i8*, i64, i64, i8*, [1 x i64], [1 x i64]} but got {{i8*, i8*, i64, i64, i8*, [1 x i64], [1 x i64]}, i8*, i8*}

ERROR: test_str_contains_regex (hpat.tests.test_hiframes.TestHiFrames)
  File "C:\Users\spokhode\AppData\Local\Continuum\anaconda3\envs\hpat-sum-env\lib\site-packages\numba\datamodel\models.py", line 696, in getter
    raise TypeError("expecting {0} but got {1}".format(*args))
TypeError: expecting {i8*, i8*, i64, i64, i8*, [1 x i64], [1 x i64]} but got {{i8*, i8*, i64, i64, i8*, [1 x i64], [1 x i64]}, i8*, i8*}

And many errors like that in different test suites:

======================================================================
FAIL: test_column_distribution (hpat.tests.test_hiframes.TestHiFrames)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\spokhode\Documents\hpat-workspace\sum-workspace\hpat\hpat\tests\test_hiframes.py", line 161, in test_column_distribution
    self.assertEqual(count_array_REPs(), 0)
AssertionError: 5 != 0

I have no idea what to do else.
@shssf, Have you any suggestion what to try or what could be wrong?

@shssf
Copy link
Contributor

shssf commented Oct 18, 2019

@PokhodenkoSA I will take a look

Implement Series.sum() in new style

Commented 'sum' in series_kernels.py series_replace_funcs list because the line is helpful and will be deleted later.
Removed kwargs, use skipna for selecting numpy.sum() or numpy.nansum(), use the same parameters for hpat_pandas_series_sum_impl
Add tests for Series.sum()
Skip test_series_sum2 because Series sum operator is not implemented
Uncomment sum in series_replace_funcs because it breaks many tests
Use numba.typing.arraydecl.ArrayAttribute.resolve_sum() instead of hpat.hiframes.pd_series_ext.SeriesAttribute.resolve_sum() in SumDummyTyper.generic() because resolve_sum was removed from SeriesAttribute
Return 'sum' in _run_call_series. It fixed count_array_REPs().
Skip Series.sum(skipna) test
Skip tests for DataFrame.sum() in new style
@PokhodenkoSA
Copy link
Contributor Author

PokhodenkoSA commented Oct 18, 2019

I have found solution (thanks to @kozlov-alexey for pointing to this).
I have returned sum to list in _run_call_series() in hpat/hiframes/hiframes_typed.py (step 2)
and uncommented 'sum' in hpat/hiframes/series_kernels.py (step 4).
Also I have skipped test for skipna because old style does not allow parameters for sum().

@shssf
Copy link
Contributor

shssf commented Oct 18, 2019

@PokhodenkoSA I started looking for this PR today and didn't understand - everything worked well.
later I found you made a changes which make this PR working :-) It is good!
Thank you for squashing commits 👍

@shssf shssf merged commit b29d0f9 into IntelPython:master Oct 18, 2019
@PokhodenkoSA
Copy link
Contributor Author

PokhodenkoSA commented Oct 18, 2019

@shssf Thank you for merging it.
BTW, GitHub can squash commits before merge https://github.blog/2016-04-01-squash-your-commits/.

@fschlimb could you please check that this feature is enabled in settings of the repository?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants