Optionally skip test requiring irlba #57
Conversation
|
If the goal is to ignore the test when irlba is not installed, why the skip is not at the first line of the test? |
|
the earlier tests seem to pass no problem without the package, so why
reduce coverage beyond what's necessary?
…On Fri, Jun 14, 2024, 1:52 AM Samuel Granjeaud ***@***.***> wrote:
If the goal is to ignore the test when irlba is not installed, why the
skip is not at the first line of the test?
—
Reply to this email directly, view it on GitHub
<#57 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB2BA5J42BMS5FNSVKBUWITZHKVOLAVCNFSM6AAAAABJJI436WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRXGU3DKOBZGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
oh, I just noticed there's no expect* unit tests before there in the same test_that call. still, some Rtsne model is fit, at least running without error is a weak test itself. |
|
Still think that the skip should be at first line. There are many tests verifying that tSNE is running correctly. This test concerns irlba, as you pointed it out. No reason to waste resource for unneeded computation. |
|
Never ran into this, since (I would hope) anyone running these tests would want to runt the irlba tests as well. For completeness, I guess it's fine to add the skip, but I agree with SamGG that it make more sense to then skip the full test, instead of still computing the reference (I think the check whether the reference runs is sufficiently covered by the other tests). |
|
IIRC, these are recent problems with the irlba package as stated in uwot also. |
|
Moved the skip. For context, we only add new packages on an as-needed basis, so Suggests are typically not brought in unless they're specifically used elsewhere -- hasn't happened (yet) for irlba, hence running into this test failure. |
|
Thanks for the clarification. The failing check is unrelated to this change. Thanks for the PR! |
No description provided.