Convert `dt.isna()` to `FExpr` #3444

samukweku · 2023-03-13T22:54:22Z

WIP for #2562

docs/api/math/isna.rst

src/core/expr/funary/isna/fexpr_isna.cc

src/datatable/math.py

tests/test-f.py

docs/api/fexpr.rst

oleksiyskononenko · 2023-03-22T06:25:38Z

The reason this build is failing may have nothing to do with this PR

[2023-03-15T18:45:15.448Z] Cancelling nested steps due to timeout

[Pipeline] // node

[Pipeline] }

[2023-03-15T18:45:15.469Z] Failed in branch Build on x86_64-macos

When you push more updates, the build will be re-triggered and we will see if this was just a temp glitch on Jenkins.

docs/api/math/isna.rst

tests/test-f.py

src/core/expr/fbinary/fexpr__eq__.cc

oleksiyskononenko · 2023-04-22T03:42:16Z

src/core/expr/fexpr_fillna.cc

+          out += ", reverse=";
+          out += reverse_? "True" : "False";
+        }


Why do we need this indent?

this section is under the else section, hence the indent so the } aligns under else. I have restored it to the previous form, however, i thought that was the idea behind indent, so any reader (me inclusive) can better understand the code (which section refers to if, else, etc, readability).

the if statement is indented, with the for loop shifted two spaces, so I only continued the form. As always, open to feedback on why this shouldnt be so. thanks @oleksiyskononenko

oleksiyskononenko · 2023-04-22T03:42:38Z

src/core/expr/fexpr_fillna.cc

+              for (size_t i = i1; i < i2; ++i) {
+                is_valid = col.get_element(i, &value);
+                fill_id = is_valid? i : fill_id;
+                indices[i] = static_cast<int32_t>(fill_id);
+              }


and this one?

same here as well, aligning the } bracket under the for

oleksiyskononenko · 2023-04-22T03:45:32Z

src/core/expr/fexpr_fillna.cc

-    }
-
-};
+ };


Strange indent too... Is it possible to set up your editor, so that it doesn't introduce these indentation changes?

src/core/column/isna.h

src/core/expr/fnary/rowcount.cc

src/core/expr/funary/fexpr_isna.cc

src/core/expr/fexpr_fillna.cc

src/core/column/isna.h

oleksiyskononenko · 2023-04-24T03:32:43Z

src/core/expr/funary/fexpr_isna.cc

+    static Column make_isna_col(Column&& col) {
+      switch (col.stype()) {
+        case SType::VOID:    return Const_ColumnImpl::make_bool_column(col.nrows(), true);
+        case SType::BOOL:
+        case SType::INT8:    return Column(new Isna_ColumnImpl<int8_t>(std::move(col)));
+        case SType::INT16:   return Column(new Isna_ColumnImpl<int16_t>(std::move(col)));
+        case SType::DATE32:
+        case SType::INT32:   return Column(new Isna_ColumnImpl<int32_t>(std::move(col)));
+        case SType::TIME64:
+        case SType::INT64:   return Column(new Isna_ColumnImpl<int64_t>(std::move(col)));
+        case SType::FLOAT32: return Column(new Isna_ColumnImpl<float>(std::move(col)));
+        case SType::FLOAT64: return Column(new Isna_ColumnImpl<double>(std::move(col)));
+        case SType::STR32:
+        case SType::STR64:   return Column(new Isna_ColumnImpl<CString>(std::move(col)));
+        default: throw RuntimeError();
+      }
+    }


Why do we want to repeat the same code here and in rowcount.cc?

how do I go about making it generic? is there a directory to store such generic function?

I have proposed Isna_ColumnImpl::make_isna_column(Column&& col) here: #3444 (comment) Seems like a relevant place.

ahh ... i missed it ... lemme have a look

I think I'll defer to U on this one @oleksiyskononenko I don't know how to implement a static method in the isna.h file and still pass the template. How would you write it?

@oleksiyskononenko I made a change to isna.h ... does that suffice? open to see how it can be improved, or done in the right way

Yeah, it's good for the moment. I will review the PR once it is green.

oleksiyskononenko · 2023-04-27T21:03:10Z

@samukweku As a part of this PR, let's not deprecate dt.isna() because it seems that it was deprecated by mistake. For the moment, let's keep both dt.isna() and dt.math.isna(). Hence, the corresponding f-method could be simply called .isna().

oleksiyskononenko · 2023-05-02T18:05:40Z

Thanks for the changes, is this PR now ready for a review?

samukweku · 2023-05-02T20:30:29Z

@oleksiyskononenko yet it is ready for a review

docs/api/frame/replace.rst

docs/api/dt/isna.rst

docs/api/fexpr.rst

docs/api/fexpr/isna.rst

docs/api/math/isna.rst

tests/math/test-isna.py

oleksiyskononenko · 2023-05-02T23:20:16Z

tests/math/test-isna.py

@@ -74,8 +74,10 @@ def test_isna_joined():
    dt.internal.frame_integrity_check(RES)
    assert RES.to_list() == [[True, True, False, True, False]] * 4

-
+@pytest.mark.xfail(reason="scalar NA check not supported")


Hm, was it ever supported? If yes, we probably need to restore it. I didn't notice we had this test.

yeah, I cant see the python fallback for this though

Then we need to restore this functionality, who knows may be it is used by some projects.

So it seems all the unary functions can take a python scalar as the argument: https://github.com/h2oai/datatable/blob/main/src/core/expr/funary/pyfn.cc#L66-L93

We need to see how to implement that with the new FExprs.

tests/munging/test-delete.py

tests/munging/test-dt-rows.py

tests/test-import-all.py

oleksiyskononenko · 2023-05-02T23:26:12Z

So I propose we keep dt.isna() as a main function, and for others (dt.math.isna() and f.isna()) say they are "the same as dt.isna()".

oleksiyskononenko · 2023-05-02T23:34:29Z

Not sure how you merge main, but it seems that you're just adding more commits on top of this branch. What you need to do is

git checkout main
git pull
git checkout samukweku/fexpr_isna
git merge main

This will create only one merge commit incorporating all the changes from main.

samukweku · 2023-05-02T23:36:22Z

i'm using the code from the guide:

git checkout main
git fetch upstream
git merge upstream/main
git checkout samukweku/fexpr_isna
git merge main

oleksiyskononenko · 2023-05-02T23:39:13Z

I guess the guide explains how to start a new branch and not how to merge existing changes onto the existing branch.

WIP for #1691

Co-authored-by: Oleksiy <[email protected]>

samukweku · 2023-05-04T12:33:46Z

@oleksiyskononenko I have made the changes. Do let me know if there are other parts that need fixing. thanks

src/datatable/math.py

src/core/expr/fexpr_isna.cc

src/core/expr/fnary/rowcount.cc

src/core/column/isna.h

oleksiyskononenko · 2023-05-04T23:12:03Z

tests/math/test-isna.py

@@ -74,8 +74,10 @@ def test_isna_joined():
    dt.internal.frame_integrity_check(RES)
    assert RES.to_list() == [[True, True, False, True, False]] * 4

-
+@pytest.mark.xfail(reason="scalar NA check not supported")


So it seems all the unary functions can take a python scalar as the argument: https://github.com/h2oai/datatable/blob/main/src/core/expr/funary/pyfn.cc#L66-L93

We need to see how to implement that with the new FExprs.

Co-authored-by: Oleksiy <[email protected]>

samukweku · 2023-05-06T12:30:03Z

@oleksiyskononenko made edits to cover for non FExprs for isna. kindly have a look when you can.

samukweku · 2023-05-27T23:05:29Z

@oleksiyskononenko just checking in on your feedback

samukweku requested a review from oleksiyskononenko March 13, 2023 22:54

samukweku self-assigned this Mar 13, 2023

oleksiyskononenko reviewed Mar 22, 2023

View reviewed changes

samukweku force-pushed the samukweku/fexpr_isna branch from db169c2 to cf4d08d Compare April 11, 2023 10:48

samukweku mentioned this pull request Apr 16, 2023

[ENH] nth function #3404

Open

oleksiyskononenko reviewed Apr 17, 2023

View reviewed changes

docs/api/math/isna.rst Outdated Show resolved Hide resolved

tests/test-f.py Outdated Show resolved Hide resolved

src/core/expr/fbinary/fexpr__eq__.cc Outdated Show resolved Hide resolved

samukweku force-pushed the samukweku/fexpr_isna branch from b339fe9 to 85909ef Compare April 20, 2023 23:29

oleksiyskononenko reviewed Apr 22, 2023

View reviewed changes

oleksiyskononenko reviewed Apr 24, 2023

View reviewed changes

oleksiyskononenko reviewed May 2, 2023

View reviewed changes

samukweku added 12 commits May 3, 2023 09:54

add header for isna

4c4b05d

implemenation of isna fexpr

c609de1

update docs

7acfade

modify docs

d69b35f

fixes

adfd351

add method for isna

629df72

add tests for isna method

f34944f

format with clang-format

4c5351c

indent

59fc3f6

update fexpr_isna

18d1f97

add to api/fexpr.rst

d30c226

fix spelling

a704458

samukweku and others added 11 commits May 3, 2023 09:54

move fexpr_isna to main folder

1c7098a

add isna to docs

ed687fe

rename doc_isna to doc_dt_isna

c7dcddf

doc_dt_isna

b32397d

Implement slicing for categorical columns (#3379)

a734bcf

WIP for #1691

Minor refactoring of methods to get the underlying column type (#3458)

f59c1b6

WIP for #1691

Update docs/api/dt/isna.rst

3df983f

Co-authored-by: Oleksiy <[email protected]>

Update docs/api/fexpr.rst

d41570f

Co-authored-by: Oleksiy <[email protected]>

Update docs/api/math/isna.rst

55efdfe

Co-authored-by: Oleksiy <[email protected]>

implemenation of isna fexpr

46535b3

update based on feedback

8856726

samukweku force-pushed the samukweku/fexpr_isna branch from 57ecf16 to 8856726 Compare May 2, 2023 23:54

fixes based on feedback

3a6d83b

oleksiyskononenko changed the title ~~[ENH] Implement FExpr for ISNA~~ Convert dt.isna() to FExpr May 4, 2023

oleksiyskononenko reviewed May 4, 2023

View reviewed changes

src/datatable/math.py Show resolved Hide resolved

oleksiyskononenko reviewed May 4, 2023

View reviewed changes

src/core/expr/fexpr_isna.cc Outdated Show resolved Hide resolved

oleksiyskononenko reviewed May 4, 2023

View reviewed changes

samukweku and others added 5 commits May 5, 2023 17:49

Update src/core/column/isna.h

b06f1f5

Co-authored-by: Oleksiy <[email protected]>

Update src/core/expr/fexpr_isna.cc

95590b6

Co-authored-by: Oleksiy <[email protected]>

remove irrelevant import

7e70e25

updates based on code review

aa2ee23

fix for isna with non fexprs

05f51ad

import isna from a single point

aae69c8

samukweku force-pushed the samukweku/fexpr_isna branch from 3d43cf3 to aae69c8 Compare May 6, 2023 22:15

samukweku added 3 commits May 7, 2023 08:23

copyright date updates

aaec02e

update copyright dates

46f53fe

Merge branch 'main' into samukweku/fexpr_isna

a193d14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `dt.isna()` to `FExpr` #3444

Convert `dt.isna()` to `FExpr` #3444

samukweku commented Mar 13, 2023 •

edited by oleksiyskononenko

Loading

oleksiyskononenko commented Mar 22, 2023

oleksiyskononenko Apr 22, 2023

samukweku Apr 23, 2023 •

edited

Loading

oleksiyskononenko Apr 22, 2023

samukweku Apr 23, 2023

oleksiyskononenko Apr 22, 2023

oleksiyskononenko Apr 24, 2023

samukweku Apr 24, 2023

oleksiyskononenko Apr 24, 2023

samukweku Apr 24, 2023

samukweku Apr 25, 2023

samukweku Apr 25, 2023 •

edited

Loading

oleksiyskononenko Apr 28, 2023

oleksiyskononenko commented Apr 27, 2023 •

edited

Loading

oleksiyskononenko commented May 2, 2023

samukweku commented May 2, 2023

oleksiyskononenko May 2, 2023

samukweku May 3, 2023

oleksiyskononenko May 4, 2023

oleksiyskononenko May 4, 2023

oleksiyskononenko commented May 2, 2023

oleksiyskononenko commented May 2, 2023

samukweku commented May 2, 2023

oleksiyskononenko commented May 2, 2023

samukweku commented May 4, 2023

oleksiyskononenko May 4, 2023

samukweku commented May 6, 2023

samukweku commented May 27, 2023

-                  }
-              };
+               };

Convert dt.isna() to FExpr #3444

Are you sure you want to change the base?

Convert dt.isna() to FExpr #3444

Conversation

samukweku commented Mar 13, 2023 • edited by oleksiyskononenko Loading

oleksiyskononenko commented Mar 22, 2023

Choose a reason for hiding this comment

samukweku Apr 23, 2023 • 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

samukweku Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleksiyskononenko commented Apr 27, 2023 • edited Loading

oleksiyskononenko commented May 2, 2023

samukweku commented May 2, 2023

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

oleksiyskononenko commented May 2, 2023

oleksiyskononenko commented May 2, 2023

samukweku commented May 2, 2023

oleksiyskononenko commented May 2, 2023

samukweku commented May 4, 2023

Choose a reason for hiding this comment

samukweku commented May 6, 2023

samukweku commented May 27, 2023

Convert `dt.isna()` to `FExpr` #3444

Convert `dt.isna()` to `FExpr` #3444

samukweku commented Mar 13, 2023 •

edited by oleksiyskononenko

Loading

samukweku Apr 23, 2023 •

edited

Loading

samukweku Apr 25, 2023 •

edited

Loading

oleksiyskononenko commented Apr 27, 2023 •

edited

Loading