Skip to content

Explore group generics implementation#365

Draft
hadley wants to merge 11 commits into
mainfrom
group-generics
Draft

Explore group generics implementation#365
hadley wants to merge 11 commits into
mainfrom
group-generics

Conversation

@hadley

@hadley hadley commented Sep 16, 2023

Copy link
Copy Markdown
Member

Fixes #353

Comment thread R/method-group.R Outdated
@hadley

hadley commented Nov 23, 2023

Copy link
Copy Markdown
Member Author

To do:

  • Actually implement the behaviour
  • Pull fallback technique from Ensure Ops falls back to base behaviour #382
  • Think about user experience. What object will they define a method for? Probably S7_Ops etc + methods::Ops
  • Where should this be documented?

@hadley

hadley commented Dec 4, 2023

Copy link
Copy Markdown
Member Author

@t-kalinowski @DavisVaughan before I finish up the implementation for the other group generics, could you please take a look at how it works for Math?

@DavisVaughan DavisVaughan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks nice to me!

Comment thread R/method-group.R
S7_error_method_not_found = function(cnd) NULL
)

NextMethod()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need to call NextMethod()? Isn't S7_object always the right-most class? It would be nice to throw the S7 method not found error if we could

Comment thread R/method-ops.R
@@ -16,17 +16,24 @@ on_load_define_ops <- function() {

#' @export
Ops.S7_object <- function(e1, e2) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this all live in method-group.R now?

Comment thread R/method-group.R
}

#' @export
Math.S7_object <- function(x, ...) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do you also need to check for a "specific" generic call here?

@t-kalinowski t-kalinowski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like a very nice solution. I see no issues.

@mmaechler

Copy link
Copy Markdown
Collaborator

Sorry if I'm a "game spoiler": Using tryCatch() in such fundamental group's methods will I think be quite a performance killer e.g. for simple arithmetic / math operations.
Is that really really necessary?

@hadley

hadley commented Dec 13, 2023

Copy link
Copy Markdown
Member Author

If you can suggest an alternative implementation, I'm happy to try it. But I couldn't think of another way to do it.

@t-kalinowski

t-kalinowski commented Dec 13, 2023

Copy link
Copy Markdown
Member

One approach might be to update method_ and method_call_ (c functions), so that, rather than than signaling an error directly, they return a sentinel value like NULL, or perhaps, the error call itself (i.e., calling eval() on the return object signals the error). Then, instead of calling S7_dispatch() in the S7_Math() generic, we could inspect for the sentinel instead. For example, S7_Math() could be defined like this:

S7_Math <<- new_generic("Math", "x", function(x, ..., .Generic) {
  cl <- .Call(method_call_, sys.call(), sys.function(), sys.frame())
  if(is.null(cl)) 
    NextMethod()
  else 
    eval(cl)
})

@mmaechler

mmaechler commented Dec 14, 2023

Copy link
Copy Markdown
Collaborator

One approach might be to update method_ and method_call_ (c functions), so that, rather than than signaling an error directly, they return a sentinel value like NULL, or perhaps, the error call itself (i.e., calling eval() on the return object signals the error). Then, instead of calling S7_dispatch() in the S7_Math() generic, we could inspect for the sentinel instead.
For example ...

"Nice"! That would almost surely be an order of magnitude faster!

Possibly we'd even want to provide (and document and use ourselves) a thin wrapper around .Call(..) such that S7-users could make use of this as well in their own method definitions.
{{I do acknowledge I haven't studied what you are doing exactly here, so "I have no clue" ;-)}}

@hadley

hadley commented Dec 14, 2023

Copy link
Copy Markdown
Member Author

Ok, I'll take a stab at that approach.

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.

Group generics

4 participants