[ENH] Refactor count/countna to use FExpr

[samukweku]

• convert count() and countna() to FExpr;
• fix countna() when applied to grouped columns;
• changeReduceUnary_ColumnImpl for more type flexibility.

WIP for #2562
Closes #3441

[samukweku]

@oleksiyskononenko i still need ur help on this PR #3404 --- if you can show me how to use FExpr_RowAll function, and also your feedback on my comments regarding skipna logic

[samukweku]

@oleksiyskononenko Also need your help in diagnosing why the tests fail in appveyor but work locally?

[oleksiyskononenko]

countna() could also be called with no cols, in such a case we are currently returning 0 for each group. Is this case covered on this PR?

[samukweku]

is it useful though?

[oleksiyskononenko]

I don't know, but may be good for consistency.

[samukweku]

yea, I dont think it fits in. we are replicating SQL here, where count(*) returns all rows, whereas count(column) counts the non null rows for that column.

[samukweku]

countna() does not mean anything

[oleksiyskononenko]

Well, but if someone already using it, it makes no sense to remove this functionality.

[oleksiyskononenko]

I don’t think anyone is using countna(), the use-case could be countna(cols), where cols could become None at some point. My feeling is that returning 0 or None is better in this case, than throwing an error.

[oleksiyskononenko]

You can do this, but then you need also to do the same for floats, etc, so better to revert the changes to what it was before this PR.

[samukweku]

made the changes - appveyor still fails - i'm particularly confused about the errors because they do not show when I run the tests locally

[oleksiyskononenko]

This failure is not related to your PR:

    def test_dt_count_na2():
        DT = dt.Frame(G=[1,1,1,2,2,2], V=[None, None, None, None, 3, 5])
        EXP = dt.Frame(G=[1,2], V1=[3,1], V2=[1,0])
>       RES = DT[:, [dt.countna(f.V), dt.countna(dt.mean(f.V))], dt.by(f.G)]
E       AssertionError: Assertion 'i < nrows()' failed in src\core\column.cc, line 251
DT         = <Frame#229a5d3da80 6x2>

You need to disable this test until #3417 is resolved.

[oleksiyskononenko]

You need to pass stype here just like it was before, otherwise it is not possible to properly get it from T_OUT. I'm not sure what was the reason to change it?

[samukweku]

the change was made here to allow passing int64 for count, instead of casting say a float to int64, as we did for the other reducers so far(sum/prod/mean). For min/max, we did not need to cast since it is within the same data type. Another option would be to pass an explicit stype here, instead of deriving the stype from T_OUT

[oleksiyskononenko]

Well, but the resulting column for count() has int64 stype, so col.stype() will work in this case too. Anyways, stype_from<T_OUT> can only give you a guess.

[samukweku]

why does stype_from<T_OUT> not work properly? maybe something I am missing from my interpretation of the function? or is Type::from d right way to go?

[oleksiyskononenko]

The way to go is to use col.stype() as it was before.

stype_from<T> will not work because there is no strict way to get stype from the underlying type. We may have different columns to have the same C++ type: date32/int32, time64/int64, boo8/int8, etc. stype_from<T> only gives you a guess.

[samukweku]

so it is ok to cast say a float64 to int64 for the count() function? no need to worry about precision?

[oleksiyskononenko]

But why do we need to cast it for count?

[oleksiyskononenko]

All reducers are separate classes, casting was needed for some of them outside of this PR. What I say is that you need to restore the original implementation for those reducers, otherwise it will not work with only passing the C++ type.

Note, tests are failing not for count…

[oleksiyskononenko]

@samukweku I guess we need to go through this PR and revert all the changes related to reducers other than count(). Only minor changes should be left that make other reducers compatible with the new ReduceUnary_ColumnImpl, i.e. calling ReduceUnary_ColumnImpl<T, T> instead of just ReduceUnary_ColumnImpl<T>.

The current way of determining the resulting column stype/casting is the way to go, and the changes on this PR are not going to work for the reasons explained above.

[oleksiyskononenko]

@samukweku I'm not sure why we need dozens of the old commits been pushed to all your PRs?

[oleksiyskononenko]

May I ask you not to push anything onto this branch? As you asked me to show you how to handle several review comments, I was going to push some changes here and suddenly got +60 old commits to deal with.

[oleksiyskononenko]

Not sure why this change was introduced, we don't need additional treatment of date32/time64 as internally there are the same as int32/int64.

[oleksiyskononenko]

@samukweku Please take a look at the changes, and let me know if you have any questions.

[samukweku]

@oleksiyskononenko my apologies; must have been when I rebased the branch. apologies on that

[oleksiyskononenko]

@samukweku To merge main, simply do git merge main being on the destination branch. It will then create just one commit and make things simple. Anyways, I guess I've resolved all the conflicts and PR is ready for your review.

[samukweku]

looks good. thanks @oleksiyskononenko . need an explanation of the code in reduceunary:

    ReduceUnary_ColumnImpl(Column &&col, const Groupby& gby)
      : ReduceUnary_ColumnImpl(std::move(col), gby, col.stype())
    {}


    ColumnImpl *clone() const override {
      return new ReduceUnary_ColumnImpl(Column(col_), Groupby(gby_), this->stype());
    }

the first part of the code gets its type directly from the column, while the other part seems to get it from the SType provided? maybe an explanation of clone and the preceding code would be helpful.

[oleksiyskononenko]

@samukweku The first part of the code you're referring to is a constructor, that is needed for the case when stype_out is the same as col.stype(). It is calling the main constructor, that accepts three parameters. This is exactly what you've been asking about, the way we need to deal with mean, min/max, etc.

[samukweku]

Thanks for the explanation @oleksiyskononenko LGTM to merge