Skip to content

Conversation

@Tuna2222
Copy link
Contributor

@Tuna2222 Tuna2222 commented Nov 12, 2025

Description

Reload the property when the user changes the properties after building the SparseSign struct.

Motivation and Context

To ensure robust usage, we have added validation and error signaling when users attempt to modify SparseSign compressor properties after initialization. This addresses the previous issue where error detection was deferred until the solver construction phase, leading to delayed and unclear feedback about invalid configurations.

How has this been tested

Try the following codes:

using RLinearAlgebra
compressor = SparseSign(compression_dim = 5, nnz = 3);
compressor.nnz = -2
compressor.nnz = 6
compressor.compression_dim = -2
compressor.compression_dim = 2

Types of changes

  • CI
  • Docs
  • Feature
  • Fix
  • Performance
  • Refactor
  • Style
  • Test
  • Other (use sparingly):

Checklists:

Code and Comments
If this PR includes modification to the code base, please select all that apply.

  • My code follows the code style of this project.
  • I have updated all package dependencies (if any).
  • I have included all relevant files to realize the functionality of the PR.
  • I have exported relevant functionality (if any).

API Documentation

  • For every exported function (if any), I have included a detailed docstring.
  • I have checked the spelling and grammar of all docstring updates through an external tool.
  • I have checked that the docstring's function signature is correctly formatted and has all arguments.
  • I have checked that the docstring's list of arguments, fields, or return values match the function.
  • I have compiled the docs locally and read through all docstring updates to check for errors.

Manual Documentation

  • I have checked the spelling and grammar of all manual updates through an external tool.
  • Any code included in the docstring is tested using doc tests to ensure consistency.
  • I have compiled the docs locally and read through all manual updates to check for errors.

Testing

  • I have added unit tests to cover my changes. (For Macros, be sure to check
    @code_lowered and
    @code_typed)
  • All new and existing tests passed.
  • I have achieved sufficient code coverage.

@Tuna2222 Tuna2222 requested review from Copilot, dmaldona, nathanielpritchard and vp314 and removed request for Copilot and vp314 November 12, 2025 17:10
@Tuna2222 Tuna2222 self-assigned this Nov 12, 2025
@Tuna2222 Tuna2222 added the bug Something isn't working label Nov 12, 2025
Copilot finished reviewing on behalf of Tuna2222 November 12, 2025 17:12
Copy link
Contributor

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 validation and error handling for field modifications on the SparseSign compressor after initialization. The implementation uses a custom setproperty! method to enforce constraints when users modify the compression_dim or nnz fields.

  • Added custom setproperty! method to validate field changes on SparseSign instances
  • Imported setproperty! from Base in the main module
  • Added comprehensive test coverage for field modification scenarios

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/RLinearAlgebra.jl Imported setproperty! from Base to support custom property setter
src/Compressors/sparse_sign.jl Added custom setproperty! method to validate compression_dim and nnz field modifications
test/Compressors/sparse_sign.jl Added test cases for valid and invalid field modifications including edge cases

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

@Tuna2222 Tuna2222 marked this pull request as ready for review November 12, 2025 17:15
@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Tuna2222 Tuna2222 merged commit 53d8a33 into main Nov 26, 2025
6 checks passed
@Tuna2222 Tuna2222 deleted the v0.2-error_throw_sparse_sign branch November 26, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants