Skip to content
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.

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

Explore group generics implementation #365

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Explore group generics implementation #365

wants to merge 11 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 16, 2023

Fixes #353

R/method-group.R Outdated Show resolved Hide resolved
@hadley
Copy link
Member Author

hadley commented Nov 23, 2023

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
Copy link
Member Author

hadley commented Dec 4, 2023

@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?

Copy link
Collaborator

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

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

Looks nice to me!

S7_error_method_not_found = function(cnd) NULL
)

NextMethod()
Copy link
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

@@ -16,17 +16,24 @@ on_load_define_ops <- function() {

#' @export
Ops.S7_object <- function(e1, e2) {
Copy link
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?

}

#' @export
Math.S7_object <- function(x, ...) {
Copy link
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?

Copy link
Member

@t-kalinowski t-kalinowski left a comment

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
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
Copy link
Member Author

hadley commented Dec 13, 2023

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
Copy link
Member

t-kalinowski commented Dec 13, 2023

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
Copy link
Collaborator

mmaechler commented Dec 14, 2023

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
Copy link
Member Author

hadley commented Dec 14, 2023

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