[REVIEW] disallow SUM and MEAN of timestamp types

[karthikeyann]

closes #4074

• disable SUM of timestamp types
• disable MEAN of timestamp types
  • column reduction
  • rolling
  • group rolling
  • unit tests
• remove DeviceProduct of timestamp types

[GPUtester]

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

[jrhemstad]

We shouldn't be allowing MEAN of timestamp at all. The user should be required to first cast to a duration or integer type.

[harrism]

We shouldn't be allowing MEAN of timestamp at all. The user should be required to first cast to a duration or integer type.

You were not so firm about this here, which may have led to this PR: #4074 (comment)

From the preceding statements, I make the following claims:

1. Trying to sum together two TIMESTAMP* types should be disallowed because the result is nonsense. Thus, the DeviceSum specialization is incorrect and should be removed.

2. Computing the MEAN of TIMESTAMP* types directly should probably be disallowed as this would require summing TIMESTAMP* types. Instead, the user should be required to convert to a duration/timedelta type(*).

I feel strongly about statement 1, but 2 could be relaxed.

[harrism]

@karthikeyann I think the first step is to add support for duration types... #5272

[karthikeyann]

We shouldn't be allowing MEAN of timestamp at all. The user should be required to first cast to a duration or integer type.

casting to duration type/integer type is a workaround.
pandas 1.0.3 implemented MEAN of timestamp without casting (#4074 (comment)). That's the main reason for this PR.

duration types will still be implemented. but it's not required for implementing MEAN of timestamp.

[jrhemstad]

pandas 1.0.3 implemented MEAN of timestamp without casting (#4074 (comment)). That's the main reason for this PR.

libcudf is not Pandas. libcudf is a C++ library that tries to follow semantics of STL types (std::chrono) as closely as possible. As such, we shouldn't allow any operation that requires summing timestamp types.

If Python wants to allow sum/mean of timestamps, then they can zero-copy cast from a timestamp to a duration/integer type and do so. But that's Python's responsibility, not libcudf's.

[nvdbaranec]

I've always found this explicit "is_mean" bool that gets passed around a little strange (the actual rolling code did it too). The logic for this can be inferred from the other parameters, ie

Suggested change

	std::enable_if_t<!is_mean_of_timestamp>* = nullptr>
	template <typename OutputType,
	typename agg_op,
	std::enable_if_t<!(agg_op == MEAN && OutputType == cudf::is_timestamp<OutputType>())>* = nullptr
	>

Then this could be bubbled up further so that is_mean can be removed from the template params for create_reference_output

[karthikeyann]

Issue #5466 is created to track this.

[nvdbaranec]

Same as above

[nvdbaranec]

Same as comment from grouped_rolling_test.cpp

[jrhemstad]

I believe this PR should be made to just disable sum of timestamp types. Emulating Pandas MEAN of timestamp can be done by casting to duration.

[harrism]

Merge after #5359 and #5394

[codecov]

Codecov Report

Merging #5319 into branch-0.15 will increase coverage by 0.29%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           branch-0.15    #5319      +/-   ##
===============================================
+ Coverage        86.09%   86.38%   +0.29%     
===============================================
  Files               75       75              
  Lines            12667    12983     +316     
===============================================
+ Hits             10906    11216     +310     
- Misses            1761     1767       +6     

Impacted Files	Coverage Δ
python/cudf/cudf/io/feather.py	100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py	0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py	0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_orc.py	100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_json.py	100.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py	100.00% <0.00%> (ø)
python/cudf/cudf/utils/applyutils.py	98.75% <0.00%> (+0.02%)	⬆️
python/cudf/cudf/core/join/join.py	92.41% <0.00%> (+0.03%)	⬆️
...ython/custreamz/custreamz/tests/test_dataframes.py	98.80% <0.00%> (+0.03%)	⬆️
python/cudf/cudf/core/frame.py	89.67% <0.00%> (+0.04%)	⬆️
... and 26 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 448f38a...53f427a. Read the comment docs.