RConsortium / S7

S7: a new OO system for R
https://rconsortium.github.io/S7
Other
386 stars 32 forks source link

Draft R dev blog post #400

Closed t-kalinowski closed 2 months ago

t-kalinowski commented 6 months ago

Draft blog post for the R development blog, detailing the recent changes in base R which enabled S7 to be published to CRAN.

(refocused iteration of #311)

ltierney commented 6 months ago

A couple of quick notes:

t-kalinowski commented 6 months ago

Thanks @ltierney!

I may be misremembering, but wasn't it only an error in Ops for both arguments to provide a method if those methods were not the same?

If two methods were found, R would use neither, issue a warning, and then proceed to try to use the built-in routines on the underlying SEXP. This is what I see in R 4.2.3:

foo <- structure(new.env(), class = "foo")
bar <- structure(new.env(), class = "bar")

`+.foo` <- function(e1, e2) "foo"
`+.bar` <- function(e1, e2) "bar"

foo + bar
Error in foo + bar : non-numeric argument to binary operator
In addition: Warning message:
Incompatible methods ("+.foo", "+.bar") for "+" 
ltierney commented 6 months ago

Thanks @ltierney!

I may be misremembering, but wasn't it only an error in Ops for both arguments to provide a method if those methods were not the same?

If two methods were found, R would use neither, issue a warning, and then proceed to try to use the built-in routines on the underlying SEXP. This is what I see in R 4.2.3:

foo <- structure(new.env(), class = "foo")
bar <- structure(new.env(), class = "bar")

`+.foo` <- function(e1, e2) "foo"
`+.bar` <- function(e1, e2) "bar"

foo + bar
Error in foo + bar : non-numeric argument to binary operator
In addition: Warning message:
Incompatible methods ("+.foo", "+.bar") for "+" 

Right, but this would not signal an error:

foo <- structure(new.env(), class = "foo")
bar <- structure(new.env(), class = "bar")

`+.foo` <- function(e1, e2) "foo"
`+.bar` <- function(e1, e2) "foo"

foo + bar
[1] "foo"
hadley commented 6 months ago

@ltierney Interesting — my memory was that would only work if you did `+.bar` <- `+.foo` (i.e. they point to the same underlying function) but I guess it actually compares the function objects.

t-kalinowski commented 6 months ago

Thanks @ltierney, I updated the chooseOpsMethod() section.

mmaechler commented 6 months ago

The %*% part seems unfinished. We did deal quite a bit with it, but are a bit in a partly finished state ... but IIRC, that is only wrt S4 which is not the topic here. I did introduce the new group "matrixOps" to which %*% belongs in 4.3. and additionally "crossprod" and "tcrossprod" in R >= 4.4.0. The fact that `%%is a _"special operator"_ ([R language definition](https://cran.r-project.org/doc/manuals/r-devel/R-lang.html#Special-operators ), also mentioned in other places in the manual) is not really crucial here. I still argue that there are functions which should get dual argument / binary dispatch (and note that there are other binary operators not inOpsnor inmatrixOps, e.g.:, or::` ...).

mmaechler commented 6 months ago

Meta comments:

t-kalinowski commented 6 months ago

Thank you @mmaechler! Regarding the meta comments:

git checkout -b R-dev-blog-post origin/R-dev-blog-post
# ... make edits to R-dev-blog-post.Rmd
git add vignettes/R-dev-blog-post.Rmd
git commit -m 'made it better'
git push

Alternatively, you can propose suggested changes in the github webui as part of a "Review"

  1. In the PR page, navigate to the "Files changed" tab. https://github.com/RConsortium/S7/pull/400/files
  2. Navigate to the line you want to edit, in the left sidebar (next to the line numbers) click the + icon that appears image
  3. Click the "Add a suggestion" button, which populates the text input area with a markdown "suggestion" cell, where you can make edits. image
  4. Click the "Start a Review" button to combine multiple such suggestions in a single batch. image

I (or others) can then click "Accept" in the webui to commit the suggestions to the branch.

Yet another alternative would be to make a new branch and open a PR from that branch to this branch, which would create a new pull request with an independent conversation thread.

lawremi commented 5 months ago

Hi @t-kalinowski, I worked on this a bit yesterday, massaging the story and some of the wording in the technical sections. I'm about halfway through and will commit soon.

lawremi commented 4 months ago

I started looking at actually posting this on the blog. One of the principles of the blog is that it should be rebuildable by anyone with relative ease. This post depends on several external packages, including the somewhat heavy tensorflow package. The examples are really powerful, so I would not want to drop them, but they add a lot of complexity to maintenance of the blog. I wonder if it would be better to create a simpler version for the dev blog and have it link to the full post hosted somewhere else? Or we could fake it by including the code output in the source file...

t-kalinowski commented 4 months ago

I think pre-rendering is the most straight-forward solution that avoids needing to add tensorflow as a dependency to the blog engine.

rmd <- "R-dev-blog-post.Rmd"
knitr::knit(rmd) -> md
# publish(md)

# or, if the blog engine *really* prefers a Rmd extension
file.rename(md, rmd)
t-kalinowski commented 3 months ago

@lawremi Would it be helpful for me to push a knitted version of the blog post?

lawremi commented 3 months ago

@t-kalinowski yea I think it would be best if you generated it, so that you can check it and make sure everything looks as expected (and you already have the dependencies installed). Sorry it took so long for me to get back to this. Thanks for your help.

t-kalinowski commented 3 months ago

Thanks @lawremi; done :heavy_check_mark:.

lawremi commented 3 months ago

@t-kalinowski I don't see the .md or any rendered chunks in the Rmd. Perhaps you forgot to add the output?

t-kalinowski commented 3 months ago

I renamed the md to an Rmd after rendering.(working on the assumption the blog engine wants an Rmd)

As the post is written, none of the chunks are meant to produce included output.

lawremi commented 3 months ago

It looks to me that many of them should produce output. For example, the first chunk should demonstrate the intended dispatch by including the output of x + Sys.date(), right? While the reader can blindly trust that things behave as expected, I think including the output would improve the overall clarity of the examples. Is there a downside?

t-kalinowski commented 3 months ago

Thanks @lawremi, good catch! It indeed makes more sense to have the chunks produce output. (I can't remember why I had set 'eval=FALSE' initially). I just pushed an updated Rmd file and knitted md file.

lawremi commented 3 months ago

This has now been committed into the blog svn. It should make it to the site with 24 hours.

t-kalinowski commented 2 months ago

The published post: https://blog.r-project.org/2024/05/17/generalizing-support-for-functional-oop-in-r/index.html This was also on the front page of Hacker News: https://news.ycombinator.com/item?id=40501561

Thanks all!