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

Optionally skip test requiring irlba #57

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

MichaelChirico
Copy link
Contributor

No description provided.

@SamGG
Copy link

SamGG commented Jun 14, 2024

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?

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Jun 14, 2024 via email

@MichaelChirico
Copy link
Contributor Author

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.

@SamGG
Copy link

SamGG commented Jun 14, 2024

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.

@jkrijthe
Copy link
Owner

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).

@SamGG
Copy link

SamGG commented Jun 14, 2024

IIRC, these are recent problems with the irlba package as stated in uwot also.

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Jun 14, 2024

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.

@jkrijthe jkrijthe merged commit 4a83e12 into jkrijthe:master Jun 14, 2024
5 of 6 checks passed
@jkrijthe
Copy link
Owner

Thanks for the clarification. The failing check is unrelated to this change. Thanks for the PR!

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

Successfully merging this pull request may close these issues.

3 participants