Skip to content

Conversation

UsmanXTech
Copy link

Fixes: #126

Description

This PR resolves the method ambiguity issue in the complete_compressor function when using the Gaussian compressor with Vector and Matrix inputs.

Changes Made

  • Added complete_compressor(ingredients::Gaussian, v::Vector{T}) to handle vectors by reshaping them into column matrices before dispatching.
  • Added complete_compressor(ingredients::Gaussian, A::AbstractMatrix) that uses invoke to call the AbstractArray method, preventing recursion and ambiguity.
  • Ensured that all existing functionality remains intact.

Motivation and Context

Previously, calling complete_compressor(::Gaussian, ::Matrix{Float64}) was ambiguous because it matched two methods:

  1. complete_compressor(ingredients::Gaussian, A::AbstractArray)
  2. complete_compressor(compressor::Compressor, A::AbstractMatrix)

This caused StackOverflowError and MethodError.
The fix ensures clear dispatch and stable behavior for Gaussian compressors when used with both Vector and Matrix inputs.

Testing

  • ✅ Full test suite passes successfully (2542/2542 tests).
  • ✅ Gaussian-specific tests now pass without errors.
  • ✅ No regressions introduced.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Code and Comments

  • My code follows the code style of this project.
  • I have included all relevant files to realize the functionality of the PR.

Testing

  • All new and existing tests passed.
  • Verified that no regressions were introduced.

- Added specific method for Vector input to reshape to column matrix
- Added method for AbstractMatrix input using invoke to resolve ambiguity
- All tests now pass (2542/2542)
Copy link
Contributor

@nathanielpritchard nathanielpritchard left a comment

Choose a reason for hiding this comment

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

We really appreciate these good suggestions. After examining your proposed solution and considering them with respect to the library, it seems that we can adapt your solution in the following way.

  1. Change the complete_compressor back to an AbstractMatrix on line 126 of the gaussian.jl file, this should eliminate the ambiguity issue that the code on line 140-143 solves, so we can also remove that code.
  2. Move lines 136-138 to line 11 in Compressors.jl, changing ingredients to compressor, Gaussian to Compressor, and vector{T} to AbstractVector.
  3. Add appropriate documentation based on the complete_compressor(::Compressor, ::AbstractMatrix).
  4. Add a test throws Argument Error for complete_compressor(::Compressor, ::AbstractVector) in ./test/Compressors/compressor_abstract_types.jl.

Thanks again for your help with this issue.

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.

2 participants