Fix: sklearn clone incompatibility in AIRS class

[himelds]

1. Added explicit p parameter to init
2. Removed usage of kwargs for parameter handling
3. Ensured no modification of parameter types (removed np.float64 conversion)
4. Implemented get_params to return only valid constructor parameters

This resolves the RuntimeError when using AIRS with sklearn pipelines.

[Joao-Paulo-Silva]

Excellent work on the pull request. The description is clear, concise, and follows the project's coding standards. Thank you for the contribution!

[himelds]

I’ve updated the implementation by moving the get_params logic to the base class using inspect.signature and removed the override from AIRS. This ensures consistent behavior across all estimators.

[Joao-Paulo-Silva]

Suggested change

	p : float
	This parameter stores the value of ``p`` used in the Minkowski distance. The default
	is ``2``, which represents normalized Euclidean distance.\
	Different values of p lead to different variants of the Minkowski Distance.

[Joao-Paulo-Silva]

Great work, and thank you for the contribution!

Before moving forward with the merge, we need to address some lint issues to ensure compliance with PEP 8 and allow the pipeline to run successfully.

Additionally, I forgot to remove the algorithm parameter from the tests in aisp/csa/tests/test_ai_recognition_sys.py. Since kwargs was removed, this will cause lint failures.

Please update lines 37, 47, and 57:

Replace:
AIRS(algorithm="binary-features", seed=seed)

With:
AIRS(seed=seed)

With these adjustments, everything should be ready for approval.

[himelds]

Thanks for your feedback!

I have addressed all the requested changes:

• Fixed the lint issues (formatting and whitespace)
• Removed the deprecated algorithm parameter from the tests
• Refactored get_params in the base class to improve compatibility
• Ensured AIRS is fully compatible with sklearn.clone by preventing internal attributes from being exposed

All tests are now passing successfully, and the implementation aligns with the expected behavior.

Please let me know if any further adjustments are needed.

[Joao-Paulo-Silva]

It looks like some additional chages were committed along with the fix. I recommend reverting to previous commit, "Refactor get_params in base class using inspect.signature for global sklearn compatibility", and only pushing the lint and docstrings fixes. This will prevent the lint and Numpydoc style checks from failing.

Here is the pipeline output for reference:

Run pylint $(git ls-files '*.py')
************* Module aisp.base.core._base
aisp/base/core/_base.py:49:0: C0304: Final newline missing (missing-final-newline)
************* Module aisp.csa._ai_recognition_sys
aisp/csa/_ai_recognition_sys.py:163:0: C0303: Trailing whitespace (trailing-whitespace)
************* Module test_ai_recognition_sys
aisp/csa/tests/test_ai_recognition_sys.py:37:16: E1123: Unexpected keyword argument 'algorithm' in constructor call (unexpected-keyword-arg)
aisp/csa/tests/test_ai_recognition_sys.py:47:16: E1123: Unexpected keyword argument 'algorithm' in constructor call (unexpected-keyword-arg)
aisp/csa/tests/test_ai_recognition_sys.py:57:16: E1123: Unexpected keyword argument 'algorithm' in constructor call (unexpected-keyword-arg)

-----------------------------------
Your code has been rated at 9.89/10

[himelds]

@Joao-Paulo-Silva, I have reverted the changes as you suggested and fixed the lint issues accordingly. Here, there is an error at the time of check. However, I noticed that the same pytest errors I encountered earlier are still occurring. That's why I made some changes to solve the issue. and the version i uploaded was fine in pytest. Could you please confirm if I should investigate and fix these test failures as well, or if there's something specific I might be missing?

Thanks!

[Joao-Paulo-Silva]

Great work!

The issue with the test lies in the fixture configuration (setup_method). Since the attributes were previously defined dynamically, the were not part of the __init__ parameters, so they were not properly recognized as parameters of the base classe.

To fix this, simply change how the fixture object is created by defining the attributes in the constructor.

In the base class test (aisp/base/core/tests/test_base.py), the fixture can be written like this, ensuring that alpha and beta are created by the class constructor:

# pylint: disable=attribute-defined-outside-init
"""Unit tests for the Base class."""

from aisp.base.core._base import Base


class TestBase:
    """Unit tests for the Base class."""

    def setup_method(self):
        """Set up a Base instance with example attributes before each test."""
        class BaseFixture(Base):
            """Fixture class for Base tests."""
            def __init__(self, alpha=1, beta=2):
                self.alpha = alpha
                self.beta = beta

        self.obj = BaseFixture()

    def test_set_and_get_params_basic(self):
        """Test setting parameters using set_params and retrieving them with get_params."""
        params_dict = {"alpha": 10, "beta": "test"}
        self.obj.set_params(**params_dict)

        params = self.obj.get_params()
        assert params == params_dict

    def test_get_params_excludes_private(self):
        """Test that get_params excludes attributes starting with an underscore."""
        self.obj._private = 123  # pylint: disable=protected-access

        params = self.obj.get_params()
        assert "_private" not in params

    def test_set_params_updates_existing(self):
        """Test that set_params updates existing attributes correctly."""
        self.obj.alpha = 5
        self.obj.set_params(alpha=42)

        assert self.obj.alpha == 42

    def test_set_params_not_hasattr(self):
        """Test that set_params ignores parameters not corresponding to existing attributes."""
        self.obj.set_params(not_params=42)

        params = self.obj.get_params()
        assert "not_params" not in params

[himelds]

@Joao-Paulo-Silva Thanks for the clarification!
I’ve updated the test fixture to define the parameters (alpha, beta) inside the constructor instead of assigning them dynamically. This ensures they are properly recognized by get_params(). Additionally, I adjusted the test case to align with the current behavior of the Base class, where only constructor-defined parameters are returned, and dynamically added attributes are excluded. All tests are now passing successfully. Please let me know if any further changes are needed.

[Joao-Paulo-Silva]

Great work, PR approved. It will be included in version 0.5.4 of the package.