feature: new tests added for tsne to expand test coverage #2229
Conversation
yuejiaointel
requested review from
Alexsandruss,
samir-nasibli and
icfaust
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
/intelci: run |
|
/intelci: run |
| tsne_perplexity = TSNE(n_components=2, perplexity=9).fit(X_perplexity) | ||
| assert tsne_perplexity.embedding_.shape == (10, 2) | ||
|
|
||
| # Test large data |
There was a problem hiding this comment.
It feels like this one is perhaps not needed, considering that there's already a similar test earlier on with shape (100,10).
There was a problem hiding this comment.
Hi David, I removed this test. Best, Yue
There was a problem hiding this comment.
Thanks, although if the "Test reproducibility" test is also checking the shape, it still feels like "Test large data" could be removed altogether since it's not testing anything different. Or does the algorithm have some size-dependent behavior that would change between (50,10) and (1000, 50)`?
…nt results, merge previous deleted gpu test to complex test
|
/intelci: run |
| False, | ||
| ), | ||
| ( | ||
| "Test reproducibility", |
There was a problem hiding this comment.
I think the reproducibility test here was lost.
| assert ( | ||
| embedding.shape == expected_shape | ||
| ), f"{description}: Incorrect embedding shape." | ||
| if device_filter == "gpu": |
There was a problem hiding this comment.
I think this doesn't need to be specific to GPU.
| tsne_perplexity = TSNE(n_components=2, perplexity=9).fit(X_perplexity) | ||
| assert tsne_perplexity.embedding_.shape == (10, 2) | ||
|
|
||
| # Test large data |
There was a problem hiding this comment.
Thanks, although if the "Test reproducibility" test is also checking the shape, it still feels like "Test large data" could be removed altogether since it's not testing anything different. Or does the algorithm have some size-dependent behavior that would change between (50,10) and (1000, 50)`?
| "description,X_generator,n_components,perplexity,expected_shape,should_raise", | ||
| [ | ||
| ( | ||
| "Test basic functionality", |
There was a problem hiding this comment.
pytest has a built-in placeholder for parametrization names - see for example here: https://docs.pytest.org/en/stable/example/parametrize.html#different-options-for-test-ids
| assert np.any( | ||
| embedding != 0 | ||
| ), f"{description}: Embedding contains only zeros." | ||
| except Exception as e: |
There was a problem hiding this comment.
This try-except could be removed after switching to pytest named parametrizations.
| ), | ||
| ( | ||
| "Edge Case: Sparse-Like High-Dimensional Data", | ||
| lambda rng: rng.random((50, 500)) * (rng.random((50, 500)) > 0.99), |
There was a problem hiding this comment.
For less-random reproducibility, perhaps it could add a test where one column is constant or an exact duplicate of another column.
|
It looks like we don't have any test here nor in daal4py that would be checking that the results from TSNE make sense beyond having the right shape and non-missingness. Since there's a very particular dataset here for the last test, it'd be helpful to add other assertions there along the lines of checking that the embeddings end up making some points closer than others as would be expected given the input data. |
Description
Added additional tests in sklearnex/manifold/tests/test_tsne.py to expand the test coverage for t-SNE algorithm.
PR completeness and readability
Testing