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

assignment of an object of class “call” is not valid for @‘.Data’ in an object of class “MethodDefinition”; is(value, "function") is not TRUE #847

Closed
kevinrue opened this issue Feb 24, 2019 · 8 comments
Labels

Comments

@kevinrue
Copy link

@kevinrue kevinrue commented Feb 24, 2019

Hi all,

I have been stumbling into the same issue for a while now, when documenting packages that include S4 methods named like primitives of the base package. I have a kind of workaround (described further below), but I would like to learn if and what is the proper way of dealing with this kind of situation.

The issue is that Roxygen crashes with the following message. As a result, the NAMESPACE file is left with only import* directives, but none of the export* directives.

==> devtools::document(roclets=c('rd', 'collate', 'namespace'))

Updating unisets documentation
Writing NAMESPACE
Loading unisets
Error in (function (cl, name, valueClass)  : 
  assignment of an object of class “call” is not valid for @‘.Data’ in an object of class “MethodDefinition”; is(value, "function") is not TRUE
Calls: suppressPackageStartupMessages ... block_find_object -> object_from_call -> parser -> <Anonymous>
In addition: Warning message:
In asNamespace(ns, base.OK = FALSE) :
  operation not allowed on base namespace
Execution halted

Exited with status 1.

This crash happens when I document functions like subset or c like this:

#' @rdname BaseSets-methods
#' @aliases subset.BaseSets subset,BaseSets-method
#'
#' @param ... Additional arguments passed to and from other methods.
#'
#' @section Subsetting:
#'
#' `subset(object, subset, ..., drop=TRUE)` returns subsets of relations which meet conditions.
#' The `subset` argument should be a logical expression referring to any of `"element"`, `"set"`, and any available relation metadata indicating elements or rows to keep: missing values are taken as false.
#' The `drop` logical scalar controls whether elements and sets orphaned during the subsetting should be removed from the `elementData` and `setData` slots, respectively.

#'
#' @importFrom methods as
#' @importFrom BiocGenerics eval unique
#' @importFrom S4Vectors from to subset
#' @method subset BaseSets
#' @export
#'
#' @examples
#'
#' bs1 <- subset(bs, set == "set1" | element == "E")
#' bs1
setMethod("subset", "BaseSets", function(x, ...) {
    .local <- function(x, subset, select, drop=TRUE, ...) {
        # Match code layout of the FuzzySets method
        table <- as.data.frame(x)
        i <- eval(substitute(subset), table)
        out <- x[i, drop=drop]
        # For derived subclasses, coerce back to the original
        as(out, class(x))
    }
    .local(x, ...)
})

I've tried the following too:

> roxygen2::roxygenize()
Loading unisets
Error in (function (cl, name, valueClass)  : 
  assignment of an object of class “call” is not valid for @‘.Data’ in an object of class “MethodDefinition”; is(value, "function") is not TRUE
In addition: Warning message:
In asNamespace(ns, base.OK = FALSE) :
  operation not allowed on base namespace

My current workaround is to define both an S3 an S4 functions, and to document the S3 only, like this:

#' @rdname BaseSets-methods
#' @aliases subset.BaseSets subset,BaseSets-method
#'
#' @param ... Additional arguments passed to and from other methods.
#'
#' @section Subsetting:
#'
#' `subset(object, subset, ..., drop=TRUE)` returns subsets of relations which meet conditions.
#' The `subset` argument should be a logical expression referring to any of `"element"`, `"set"`, and any available relation metadata indicating elements or rows to keep: missing values are taken as false.
#' The `drop` logical scalar controls whether elements and sets orphaned during the subsetting should be removed from the `elementData` and `setData` slots, respectively.

#'
#' @importFrom methods as
#' @importFrom BiocGenerics eval unique
#' @importFrom S4Vectors from to subset
#' @method subset BaseSets
#' @export
#'
#' @examples
#'
#' bs1 <- subset(bs, set == "set1" | element == "E")
#' bs1
subset.BaseSets <- function(x, ...) subset(x, ...)

setMethod("subset", "BaseSets", function(x, ...) {
    .local <- function(x, subset, select, drop=TRUE, ...) {
        # Match code layout of the FuzzySets method
        table <- as.data.frame(x)
        i <- eval(substitute(subset), table)
        out <- x[i, drop=drop]
        # For derived subclasses, coerce back to the original
        as(out, class(x))
    }
    .local(x, ...)
})

Can anyone spot what I'm doing wrong?
The package source code is hosted here: https://github.com/kevinrue/unisets if anyone is willing to open a PR showing me the fix.

Thank you in advance!

@hadley hadley added bug reprex and removed bug labels Jul 20, 2019
@hadley
Copy link
Member

@hadley hadley commented Jul 26, 2019

If I convert this into the simplest possible reprex, it seems to work ok:

library(roxygen2)
roc_proc_text(namespace_roclet(), "
  setClass('Foo')

  #' @export
  setMethod('subset', 'Foo', function(x, ...) {
    10
  })
")
#> [1] "exportMethods(subset)"

Can you please have a go at making this more realistic to reveal your problem? This process will either reveal what you're doing wrong, or where the problem with roxygen2 lines.

@kevinrue
Copy link
Author

@kevinrue kevinrue commented Jul 26, 2019

Thanks @hadley for replying. Genuinely appreciated.
I'm sorry that I've left this aside for a while now. My code is still pretty much in the same state: https://github.com/kevinrue/unisets/blob/master/R/Sets-class.R#L325

To be honest, I'm still a little bit confused whether it's even necessary to write both S3 and S4 methods in this kind of scenario.
Also, I'm not sure what you mean by "realistic". All I wanted to do at the time is to implement a subset method following best - or at least good - practice.
Consider how subset is a base method, while I wrote an S4 class, I wasn't sure whether to implement an S3 or S4 method, or both.

Lastly I don't want to waste too much of anyone's time, as I will point out that there is effort to develop a similar package in a "tidy" style by the Bioconductor core team here: https://github.com/Kayla-Morrell/BiocSet

Again, this was all mostly to teach myself good practice in terms of package development. I'm mostly self taught and I didn't know where to turn to learn this particular S3/S4 point.

@hadley
Copy link
Member

@hadley hadley commented Jul 27, 2019

You should not need to write S3 and S4 methods. All I'm looking for is a reprex in the style above, the demonstrates the problem that you are seeing.

@kevinrue
Copy link
Author

@kevinrue kevinrue commented Jul 28, 2019

Here we go:

library(roxygen2)
roc_proc_text(namespace_roclet(), "
  setClass('Foo')

  #' @export
  setMethod('subset', 'Foo', function(x, ...) {
    .local <- function(x, subset, select, drop=TRUE, ...) {
      11
    }
    .local(x, ...)
  })
")
#> Error in (function (cl, name, valueClass) : assignment of an object of class "call" is not valid for @'.Data' in an object of class "MethodDefinition"; is(value, "function") is not TRUE

Seems to be due to function defined within other functions. Bad practice? I know I've picked that up from somewhere, I just can't find from where right now 😅

@kevinrue kevinrue closed this Jul 28, 2019
@kevinrue kevinrue reopened this Jul 28, 2019
@kevinrue
Copy link
Author

@kevinrue kevinrue commented Jul 28, 2019

(sorry, I didn't mean to close the issue - accidental keyboard shortcut did that)

@hadley
Copy link
Member

@hadley hadley commented Jul 29, 2019

Ah, it looks like you’re copying something that S4 does automatically in some cases. I don’t think there’s any need for it here, so you should remove it, and I’ll also make a better error message here.

@kevinrue
Copy link
Author

@kevinrue kevinrue commented Jul 29, 2019

Thanks for the feedback.
It actually helped me figure out where I found this "coding style" in the first place.

E.g. For my own record in the S4Vectors package
https://github.com/Bioconductor/S4Vectors/blob/1a3b937784e7d26158b268a751474891a32baf57/R/Vector-class.R#L575

The source code below

subset_Vector <- function(x, subset, select, drop=FALSE, ...)
{
    ## Fix old objects on-the-fly (e.g. old GRanges, GRangesList, or
    ## GAlignments instances).
    x <- updateObject(x, check=FALSE)
    i <- evalqForSubset(subset, x, ...)
    x_mcols <- mcols(x, use.names=FALSE)
    if (!is.null(x_mcols)) {
        j <- evalqForSelect(select, x_mcols, ...)
        mcols(x) <- x_mcols[ , j, drop=FALSE]
    }
    x[i, drop=drop]
}
setMethod("subset", "Vector", subset_Vector)

is automatically turned by S4 into the following code:

> showMethods("subset", includeDefs = T, classes = "Vector")
Function: subset (package base)
x="Vector"
function (x, ...) 
{
    .local <- function (x, subset, select, drop = FALSE, ...) 
    {
        i <- evalqForSubset(subset, x, ...)
        x_mcols <- mcols(x, use.names = FALSE)
        if (!is.null(x_mcols)) {
            j <- evalqForSelect(select, x_mcols, ...)
            mcols(x) <- x_mcols[, j, drop = FALSE]
        }
        x[i, drop = drop]
    }
    .local(x, ...)
}

I clearly used the latter as template for writing my own source code.
I didn't realize that S4 was doing automatically this kind of conversion. Magic!

@hadley hadley added bug and removed reprex labels Aug 12, 2019
@hadley
Copy link
Member

@hadley hadley commented Aug 12, 2019

Looks like the code never did what it was supposed to (i.e. automatically extracting the S4 generated .local()) so thanks for reporting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants