Bioconductor / MatrixGenerics

S4 Generic Summary Statistic Functions that Operate on Matrix-Like Objects
https://bioconductor.org/packages/MatrixGenerics
12 stars 1 forks source link

Argument 'center' package tests #20

Closed HenrikBengtsson closed 3 years ago

HenrikBengtsson commented 3 years ago

With the new 'develop' branch matrixStats version, I know get:

...
   1. ├─MatrixGenerics::colWeightedMads(...) test-api_compatibility.R:698:8
   2. └─MatrixGenerics::colWeightedMads(...)
   3.   └─matrixStats::colWeightedMads(...)

  ── ERROR (test-api_compatibility.R:1466:2): rowWeightedMads works  ─────────────
  Error: Argument 'center' should be of the same length as number of rows of 'x': 1 != 6
  Backtrace:
      █
   1. ├─MatrixGenerics::rowWeightedMads(...) test-api_compatibility.R:1466:8
   2. └─MatrixGenerics::rowWeightedMads(...)
   3.   └─matrixStats::rowWeightedMads(...)

  ══ testthat results  ═══════════════════════════════════════════════════════════
  ERROR (test-api_compatibility.R:593:2): colSds works 
  ERROR (test-api_compatibility.R:678:2): colVars works 
  ERROR (test-api_compatibility.R:698:2): colWeightedMads works 
  ERROR (test-api_compatibility.R:1466:2): rowWeightedMads works 

  [ FAIL 4 | WARN 0 | SKIP 0 | PASS 306 ]
  Error: Test failures
  Execution halted

This is related to https://github.com/HenrikBengtsson/matrixStats/issues/183. I've deprecated length(center) == 1 but for now it's still valid when arguments rows and cols are not specified. I think the above these use does the latter.

You could test with center <- rep(center, times = nrow(x) to avoid this. However, when you do, you must make sure that you test with a valid center estimate because otherwise the new sanity checks related to Issue #160 (switching from "alternative" to "primary" formula for the sample variance estimator).

hpages commented 3 years ago

Why deprecate length(center) == 1? The only interesting use of rowVars(x, center=my_center) that I'm aware of, besides center=rowMeans(x), is center=0. Not sure why people would be required to do rowVars(x, center=rep(0, times=nrow(x))) when they could just do rowVars(x, center=0).

HenrikBengtsson commented 3 years ago

Why deprecate ...

... to get feedback like this :p So, maybe center = 0 should be a special case then, but only that one. Or?

In the bigger picture and related to https://github.com/HenrikBengtsson/matrixStats/issues/187, this comes down to what the true identity of center should be. Right now, it serves different purposes and it has caused problems. Tighten up what's allowed and not helps to get to the true identity.

HenrikBengtsson commented 3 years ago

And regarding the special case of center = 0, does that apply to all other functions that take argument center or only for {col,row}Vars()? What about the weighted versions?

const-ae commented 3 years ago

I fixed the tests for MatrixGenerics. They don't use the scalar center argument anymore, but a proper estimate of the mean for colVars, colSds, colWeightedVars, and colWeightedSds.

As far as I can see the best place to continue the discussion around center = 0, is at https://github.com/HenrikBengtsson/matrixStats/issues/193. So I will close this issue for now :)

But feel free to reopen, if I missed something.