Skip to content

Attempt S4 dispatch for intersect() and union(), maybe #305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MichaelChirico wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Attempt S4 dispatch for intersect() and union(), maybe #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9c0eedb
Attempt S4 dispatch for intersect(), maybe
MichaelChirico Mar 19, 2026
f9ec364
rearrange
MichaelChirico Mar 19, 2026
7f4902a
corresponding test
MichaelChirico Mar 19, 2026
9b5a9e7
nanotime uses union(), too
MichaelChirico Mar 19, 2026
d556d21
tests for union
MichaelChirico Mar 19, 2026
8d07b7b
(rm unneeded)
MichaelChirico Mar 19, 2026
cf211e9
also setdiff
MichaelChirico Mar 19, 2026
e11898f
setdiff tests
MichaelChirico Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,8 @@ importFrom(graphics,par)
importFrom(graphics,title)
importFrom(methods,as)
importFrom(methods,is)
importFrom(methods,isGeneric)
importFrom(methods,selectMethod)
importFrom(stats,cor)
importFrom(stats,median)
importFrom(stats,quantile)
Expand Down
2 changes: 1 addition & 1 deletion R/bit64-package.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@
#' ramsort ramsortorder repeat.time setattr shellorder shellsort
#' shellsortorder still.identical
#' @importFrom graphics barplot par title
#' @importFrom methods as is
#' @importFrom methods as is isGeneric selectMethod
#' @importFrom stats cor median quantile
#' @importFrom utils head packageDescription strOptions tail getS3method
#' @export : %in% is.double match order print.cache rank
Expand Down
16 changes: 14 additions & 2 deletions R/setops64.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,13 @@
#' @export
#' @rdname sets
union = function(x, y) {
if ((isS4(x) || isS4(y)) && isGeneric("union")) {
s4_intersect = selectMethod("union", list(x=class(x), y=class(y)))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
s4_intersect = selectMethod("union", list(x=class(x), y=class(y)))
s4_union = selectMethod("union", list(x=class(x), y=class(y)))

Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

since we are in union

return(s4_intersect(x, y))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
return(s4_intersect(x, y))
return(s4_union(x, y))

}
if (!(is.integer64(x) || is.integer64(y)))
return(base::union(x, y))

target_class = target_class(list(x, y))
# try using the benefit of integer64 caching, if possible. I.e. call unique() before as().
x = unique(x)
Expand All @@ -60,9 +64,13 @@ union = function(x, y) {
#' @rdname sets
intersect = function(x, y) {
if (is.null(x) || is.null(y)) return(NULL)
if ((isS4(x) || isS4(y)) && isGeneric("intersect")) {
s4_intersect = selectMethod("intersect", list(x=class(x), y=class(y)))
return(s4_intersect(x, y))
}
if (!(is.integer64(x) || is.integer64(y)))
return(base::intersect(x, y))

target_class = target_class(list(x, y))
x = unique(x)
class_x = class(x)[1L]
Expand Down Expand Up @@ -109,6 +117,10 @@ setequal = function(x, y) {
#' @export
#' @rdname sets
setdiff = function(x, y) {
if ((isS4(x) || isS4(y)) && isGeneric("setdiff")) {
s4_intersect = selectMethod("setdiff", list(x=class(x), y=class(y)))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
s4_intersect = selectMethod("setdiff", list(x=class(x), y=class(y)))
s4_setdiff = selectMethod("setdiff", list(x=class(x), y=class(y)))

return(s4_intersect(x, y))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
return(s4_intersect(x, y))
return(s4_setdiff(x, y))

}
if (!(is.integer64(x) || is.integer64(y)))
return(base::setdiff(x, y))

Expand Down
52 changes: 52 additions & 0 deletions tests/testthat/test-setops64.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,3 +320,55 @@ test_that("is.element works (additional cases)", {
expect_true(is.element(NA_integer64_, NA))

})
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
})
})
test_that("S3 dispatch happens for classes extending integer64 (#298)", {
x = 1:2
y = as.integer64(2:3)
class(y) = c('foo', 'integer64')
expect_identical(intersect(x, y), as.integer64(2L))
expect_identical(union(x, y), as.integer64(1:3))
expect_identical(setdiff(x, y), 1L)
})

Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Maybe add some integer64 extended S3 dispatch test


test_that("S4 dispatch still happens for classes extending integer64 (#301)", {
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
test_that("S4 dispatch still happens for classes extending integer64 (#301)", {
# This test has to be the last test, because it adds a generic (e.g. intersect), which cannot be removed properly during R CMD CHECK.
# If removeGeneric() is called the following test have base::intersect instead of bit64::intersect as 'generic'.
# This even applies for the two runs per 'method' within the test.
with_parameters_test_that("S4 and S3 dispatch still happens for classes extending {dataType} (#301)", {
x = as(1:2, dataType)
y = as.integer64(2:3)
# S4
methods::setClass("TestS4", representation(data=dataType))
methods::setGeneric(method)
methods::setMethod(
method,
signature=c("TestS4", "integer64"),
function(x, y) "Successfully routed to S4 method A!"
)
methods::setMethod(
method,
signature=c("TestS4", "TestS4"),
function(x, y) "Successfully routed to S4 method B!"
)
# Instantiate test objects
xS4 = methods::new("TestS4", data=x)
fun = get(method)
expect_identical(fun(xS4, y), "Successfully routed to S4 method A!")
# NB: nanoival class is "complex64" -- it kludges complex to be a pair
# of integer64 vectors, but there is no complex64 class, so it just
# shows up on the inheritance chain as 'complex' --> need to ensure
# S4 gets invoked when possible even if the inputs don't directly test
# as being is("integer64").
expect_identical(fun(xS4, xS4), "Successfully routed to S4 method B!")
# S3
yS3 = y
class(yS3) = c('foo', 'integer64')
actual_result = fun(x, yS3)
expected_result = fun(as.integer(x), as.integer(y))
if (!(dataType == "integer" && method == "setdiff"))
expected_result = as.integer64(expected_result)
expect_identical(actual_result, expected_result)
# cleanup
methods::removeMethod(method, signature=c("TestS4", "integer64"))
methods::removeMethod(method, signature=c("TestS4", "TestS4"))
methods::removeClass("TestS4")
},
.cases=expand.grid(
dataType=c("integer", "integer64"),
method=c("intersect", "union", "setdiff"),
stringsAsFactors=FALSE
)
)

Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

I would change the test to one with parameters and extend it with a S3 dispatch when a S4 generic exists.

methods::setClass("TestS4", representation(data="integer"))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
methods::setClass("TestS4", representation(data="integer"))

Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Remove the remains of the original test line by line.


Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change

delete_intersect_generic = !methods::isGeneric("intersect")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
delete_intersect_generic = !methods::isGeneric("intersect")

suppressMessages(methods::setGeneric("intersect"))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
suppressMessages(methods::setGeneric("intersect"))

delete_union_generic = !methods::isGeneric("union")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
delete_union_generic = !methods::isGeneric("union")

suppressMessages(methods::setGeneric("union"))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
suppressMessages(methods::setGeneric("union"))

delete_setdiff_generic = !methods::isGeneric("setdiff")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
delete_setdiff_generic = !methods::isGeneric("setdiff")

suppressMessages(methods::setGeneric("setdiff"))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
suppressMessages(methods::setGeneric("setdiff"))


Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change

methods::setMethod(
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
methods::setMethod(

"intersect",
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
"intersect",

signature=c("TestS4", "integer64"),
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
signature=c("TestS4", "integer64"),

function(x, y) "Successfully routed to S4 method!"
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
function(x, y) "Successfully routed to S4 method!"

)
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
)

methods::setMethod(
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
methods::setMethod(

"union",
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
"union",

signature=c("TestS4", "integer64"),
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
signature=c("TestS4", "integer64"),

function(x, y) "Successfully routed to S4 method!"
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
function(x, y) "Successfully routed to S4 method!"

)
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
)

methods::setMethod(
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
methods::setMethod(

"setdiff",
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
"setdiff",

signature=c("TestS4", "integer64"),
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
signature=c("TestS4", "integer64"),

function(x, y) "Successfully routed to S4 method!"
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
function(x, y) "Successfully routed to S4 method!"

)
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
)

withr::defer({
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
withr::defer({

methods::removeMethod("setdiff", signature=c("TestS4", "integer64"))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
methods::removeMethod("setdiff", signature=c("TestS4", "integer64"))

methods::removeMethod("union", signature=c("TestS4", "integer64"))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
methods::removeMethod("union", signature=c("TestS4", "integer64"))

methods::removeMethod("intersect", signature=c("TestS4", "integer64"))
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
methods::removeMethod("intersect", signature=c("TestS4", "integer64"))

methods::removeClass("TestS4")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
methods::removeClass("TestS4")

if (delete_setdiff_generic) methods::removeGeneric("setdiff")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
if (delete_setdiff_generic) methods::removeGeneric("setdiff")

if (delete_union_generic) methods::removeGeneric("union")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
if (delete_union_generic) methods::removeGeneric("union")

if (delete_intersect_generic) methods::removeGeneric("intersect")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
if (delete_intersect_generic) methods::removeGeneric("intersect")

})
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
})


Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change

# Instantiate test objects
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
# Instantiate test objects

x = methods::new("TestS4", data = 1L)
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
x = methods::new("TestS4", data = 1L)

y = as.integer64(2L)
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
y = as.integer64(2L)


Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change

expect_identical(intersect(x, y), "Successfully routed to S4 method!")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
expect_identical(intersect(x, y), "Successfully routed to S4 method!")

expect_identical(union(x, y), "Successfully routed to S4 method!")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
expect_identical(union(x, y), "Successfully routed to S4 method!")

expect_identical(setdiff(x, y), "Successfully routed to S4 method!")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
expect_identical(setdiff(x, y), "Successfully routed to S4 method!")

# NB: nanoival class is "complex64" -- it kludges complex to be a pair
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
# NB: nanoival class is "complex64" -- it kludges complex to be a pair

# of integer64 vectors, but there is no complex64 class, so it just
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
# of integer64 vectors, but there is no complex64 class, so it just

# shows up on the inheritance chain as 'complex' --> need to ensure
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
# shows up on the inheritance chain as 'complex' --> need to ensure

# S4 gets invoked when possible even if the inputs don't directly test
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
# S4 gets invoked when possible even if the inputs don't directly test

# as being is("integer64").
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
# as being is("integer64").

expect_identical(intersect(x, x), "Successfully routed to S4 method!")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
expect_identical(intersect(x, x), "Successfully routed to S4 method!")

expect_identical(union(x, x), "Successfully routed to S4 method!")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
expect_identical(union(x, x), "Successfully routed to S4 method!")

expect_identical(setdiff(x, x), "Successfully routed to S4 method!")
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
expect_identical(setdiff(x, x), "Successfully routed to S4 method!")

})
Copy link
Copy Markdown
Collaborator

@hcirellu hcirellu Mar 24, 2026

Choose a reason for hiding this comment

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

Suggested change
})

Loading