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

Argument 'center' package tests #20

Closed
HenrikBengtsson opened this issue Nov 26, 2020 · 4 comments
Closed

Argument 'center' package tests #20

HenrikBengtsson opened this issue Nov 26, 2020 · 4 comments

Comments

@HenrikBengtsson
Copy link

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 HenrikBengtsson/matrixStats#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
Copy link
Contributor

hpages commented Nov 26, 2020

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

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 HenrikBengtsson/matrixStats#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
Copy link
Author

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 added a commit to const-ae/MatrixGenerics that referenced this issue Dec 6, 2020
Use `center` argument correctly in tests
@const-ae
Copy link
Collaborator

const-ae commented Dec 6, 2020

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 HenrikBengtsson/matrixStats#193. So I will close this issue for now :)

But feel free to reopen, if I missed something.

@const-ae const-ae closed this as completed Dec 6, 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

3 participants