Skip to content

Conversation

Copy link

Copilot AI commented Dec 18, 2025

Addresses review feedback on PR #88 by adding comprehensive test coverage for sparse matrices and edge cases in random landmarking with precomputed matrices.

Changes

  • Sparse precomputed affinity test: Validates CSR sparse affinity matrices trigger the correct code path (lines 1210-1217 in graphs.py)
  • Sparse precomputed distance test: Validates CSR sparse distance matrices work correctly through distance-to-affinity conversion
  • Zero affinity edge case test: Validates RuntimeWarning is raised when samples have zero affinity to all randomly selected landmarks (tests lines 1223-1228 in graphs.py)
  • Comment clarity improvement: Updated graphs.py:1207 to clarify that kernel contains affinities regardless of precomputed type
# Now tested with sparse matrices
affinity_sparse = sp.csr_matrix(affinity)
G = graphtools.Graph(
    affinity_sparse,
    precomputed="affinity",
    n_landmark=3,
    random_landmarking=True,
)

All 21 random landmarking tests pass.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…prove comment clarity

Co-authored-by: MattScicluna <19255250+MattScicluna@users.noreply.github.com>
Copilot AI changed the title [WIP] Add random landmarking support for precomputed distance and affinity Add sparse matrix and edge case test coverage for random landmarking with precomputed matrices Dec 18, 2025
Copilot AI requested a review from MattScicluna December 18, 2025 19:37
@coveralls
Copy link

Pull Request Test Coverage Report for Build 20349013595

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 20348469484: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

Copy link
Contributor

@MattScicluna MattScicluna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code lgtm. But not sure why coveralls failing.

@MattScicluna MattScicluna marked this pull request as ready for review December 18, 2025 19:43
Copilot AI review requested due to automatic review settings December 18, 2025 19:43
@MattScicluna MattScicluna merged commit eb30e4a into random_landmarking_precomp_aff Dec 18, 2025
11 of 14 checks passed
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds comprehensive test coverage for sparse matrices and edge cases in random landmarking with precomputed matrices. The changes address review feedback from PR #88 by ensuring that sparse precomputed affinity/distance matrices and zero-affinity edge cases are properly tested.

Key Changes:

  • Added test for sparse CSR precomputed affinity matrices with random landmarking
  • Added test for sparse CSR precomputed distance matrices with random landmarking
  • Added test for zero affinity edge case that validates proper warning emission
  • Improved code comment clarity in graphs.py to better explain kernel usage

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
test/test_random_landmarking.py Added three new test functions covering sparse precomputed matrices (affinity and distance) and zero affinity warning edge case, plus sp import
graphtools/graphs.py Updated comment to clarify that kernel contains affinities computed from precomputed matrices regardless of precomputed type

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +593 to +608
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
G = graphtools.Graph(
affinity,
precomputed="affinity",
n_landmark=n_landmark,
random_landmarking=True,
random_state=random_state,
knn=3,
thresh=0,
)
_ = G.landmark_op

assert len(w) == 1
assert issubclass(w[0].category, RuntimeWarning)
assert "zero affinity to all randomly selected landmarks" in str(w[0].message)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test should use the assert_warns_message helper function available in load_tests instead of manual warning capture with warnings.catch_warnings. This is the established pattern in the codebase for testing warnings (see test_exact.py line 96). Replace this manual warning handling with a cleaner approach using the helper.

Copilot uses AI. Check for mistakes.
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