Skip to content

New handling of expressions in data_modify(), introducing as_expr() helper function#605

Merged
strengejacke merged 83 commits intomainfrom
data_modify_adds_values
Apr 29, 2025
Merged

New handling of expressions in data_modify(), introducing as_expr() helper function#605
strengejacke merged 83 commits intomainfrom
data_modify_adds_values

Conversation

@strengejacke
Copy link
Copy Markdown
Member

@strengejacke strengejacke commented Apr 10, 2025

library(datawizard)

data_modify(iris, as_expression("sepwid = 2 * Sepal.Width")) |> head()

data_modify(
    iris,
    as_expression(c("sepwid = 2 * Sepal.Width", "seplen = 5 * Sepal.Length"))
) |> head()

data_modify(iris, sepwid = as_expression("2 * Sepal.Width")) |> head()

data_modify(
    iris,
    sepwid = as_expression("2 * Sepal.Width"),
    seplen = {"5 * Sepal.Length"}
) |> head()

e <- "sepwid = 2 * Sepal.Width"
data_modify(iris, as_expression(e)) |> head()

e <- c("sepwid = 2 * Sepal.Width", "seplen = 5 * Sepal.Length")
data_modify(iris, as_expression(e)) |> head()

e <- "2 * Sepal.Width"
data_modify(iris, sepwid = as_expression(e)) |> head()

e <- "2 * Sepal.Width"
data_modify(iris, sepwid = as_expression(e), seplen = 5 * Sepal.Length) |> head()

e <- "2 * Sepal.Width"
f <- "half_petal = 0.5 * Petal.Length"
a <- "string"
num <- 1:5
data_modify(
    iris,
    sepwid = as_expression(e),
    seplen = 5 * Sepal.Length,
    {f},
    new_var = a,
    new_num = num,
    new_var2 = "ho",
    new_num2 = 4:6,
    Sepal.Length = NULL,
    Petal.Length = NULL,
    Sepal.Width = NULL,
    Petal.Width = NULL
) |> head()


d <- data.frame()
for (param in letters[c(1, 2, 5)]) {
  out <- data.frame(x = as.numeric(as.factor(param)))
  out <- data_modify(out, Parameter = param)
  d <- rbind(out, d)
}
d

data_modify(iris, sepwid = "2 * Sepal.Widht") |> head()

# errors

e <- "sepwid = 2 * Sepal.Widht"
data_modify(iris, as_expression(e))

data_modify(iris, as_expression("sepwid = 2 * Sepal.Widht"))

data_modify(iris, sepwid = 2 * Sepal.Widht)

@etiennebacher

This comment was marked as outdated.

@strengejacke

This comment was marked as outdated.

@strengejacke

This comment was marked as outdated.

@strengejacke

This comment was marked as outdated.

@etiennebacher etiennebacher mentioned this pull request Apr 22, 2025
@strengejacke

This comment was marked as outdated.

@strengejacke

This comment was marked as outdated.

@strengejacke
Copy link
Copy Markdown
Member Author

Thanks for the review! All comments are resolved, hence now we have additional features included. Tests should be added for all cases.

@strengejacke strengejacke changed the title New handling of expressions in data_modify(), introducing as_expression() helper function New handling of expressions in data_modify(), introducing as_expr() helper function Apr 29, 2025
Copy link
Copy Markdown
Member

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Thanks, almost there for me. The implementation is quite complex but the test suite looks complete and I like the fact that one can still build and evaluate expressions from strings.

@DominiqueMakowski does the current behavior look ok to you?

strengejacke and others added 7 commits April 29, 2025 22:08
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@strengejacke

This comment was marked as outdated.

@etiennebacher

This comment was marked as outdated.

@strengejacke
Copy link
Copy Markdown
Member Author

I guess we're going in this direction now, but let's keep it for another PR, this one is already very dense.

Maybe let's wait a bit. In other functions, by is unique, but here it could be a new variable name, conflicting with the by argument. I guess we need to think more about this for data_modify().

@strengejacke
Copy link
Copy Markdown
Member Author

Ok, I think recent comments are also resolved.

Copy link
Copy Markdown
Member

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Alright, thanks for addressing all the comments, I'm happier with the current implementation than the old one. We have 3 weeks to fix CRAN errors so let's merge this now and revisit later if some things must be improved.

@strengejacke
Copy link
Copy Markdown
Member Author

strengejacke commented Apr 29, 2025

CRAN errors should already be fixed, I think. You can submit any time, but I think you had some reminders/todos in #615 before submission.

Let me wait for checks, then I'll merge.,

@strengejacke strengejacke merged commit 0265ef7 into main Apr 29, 2025
24 of 25 checks passed
@strengejacke strengejacke deleted the data_modify_adds_values branch April 29, 2025 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants