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

Seems like a bad idea to coerce a DelayedMatrix to the seed class #63

Closed
LTLA opened this issue Sep 25, 2020 · 0 comments
Closed

Seems like a bad idea to coerce a DelayedMatrix to the seed class #63

LTLA opened this issue Sep 25, 2020 · 0 comments

Comments

@LTLA
Copy link
Contributor

LTLA commented Sep 25, 2020

I noticed that colVars (and possibly other functions) has this little nugget:

message2("Coercing to seed class", get_verbose())
# TODO: do_transpose trick
simple_seed_x <- try(from_DelayedArray_to_simple_seed_class(x),
silent = TRUE)

I'm not sure this is a good idea; does this respect the block size and memory constraints of block processing? For example, if I defined something like this:

library(DelayedArray) 
y <- DelayedArray(rsparsematrix(10000, 10000, density=0.1)) + 1
 
library(DelayedMatrixStats) 
library(sparseMatrixStats) 
colVars(y)

If seedClass() was smarter than it currently is and detected that the dgCMatrix at the core of y now has a colVars() method, the function would attempt to realize the dense matrix in the above example - not good. Fortunately, seedClass() is not very smart and returns DelayedUnaryIsoOpStack, which means that we always go through block processing. This means that either seedClass()'s lack of intelligence is intentional, in which case it seems like isPristine() will always be true at that point in the function and there is no point in the else expression; or it is meant to be more intelligent, in which case memory constraints are likely to be violated.

Why not just have a really simple rule:

    if (isPristine(x)) {
        candidate <- selectMethod("colVars", seed(x), optional=TRUE)
        if (!is.null(candidate)) {
            return(candidate(seed(x), ...)) # fill in the other arguments...
        }
    }

    # block processing here.
LTLA added a commit to LTLA/DelayedMatrixStats that referenced this issue Sep 28, 2020
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

No branches or pull requests

1 participant