🦙 MegaLinter status: ✅ SUCCESS

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by

Codecov Report

Merging #438 (dd6f4d1) into main (2e59d4b) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #438   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           48        49    +1     
  Lines         2514      2594   +80     
=========================================
+ Hits          2514      2594   +80     

@Marsmaennchen221

To make the error messages somewhat more user-friendly, I have prefixed the NotImplementedErrors with Illegal interval: .
Also changed them to ValueErrors, see below.
Also, we might even include the specific parameters in the error message, e.g. something like: Trying to raise OutOfBoundsError, but 0 is actually inside (-1,1).

Edit: All done.

In _k_nearest_neighbors.py:

        if self._number_of_neighbors > training_set.number_of_rows:
            raise ValueError(
                (
                    f"The parameter 'number_of_neighbors' ({self._number_of_neighbors}) has to be less than or equal to"
                    f" the sample size ({training_set.number_of_rows})."
                ),
            )

(Same in the regression version.)

We can replace this with the new OutOfBoundsError, but then we cannot customize the error message as much. (It does allow for a name keyword argument, so we can tell the user the name of the number_of_neighbors parameter, but we cannot talk about the "sample size" anymore.)

We could make the OutOfBoundsError more flexible to also allow passing arbitrary strings to it (for example with a new custom_message: str keyword argument).
But then the error itself gets somewhat complicated to use...

Is it in scope of this PR to also adjust the bounds checks in other places (e.g. number_of_trees in RandomForest)? If not, there should be an issue for that.

I did update a few cases in _image.py as there was already a comment about it in the issue. Guess I can also check for any other cases where this makes sense and update them.
Converting back to draft meanwhile.

Thanks. I think only some hyperparameter of models should be affected. Methods like Table.get_row that raise an IndexOutOfBoundsError can be kept unchanged.

Edit: An the number_of_bins on Discretizer.

Changed all ML models (and the discretizer) to use the new error.

In _k_nearest_neighbors.py:

        if self._number_of_neighbors > training_set.number_of_rows:
            raise ValueError(
                (
                    f"The parameter 'number_of_neighbors' ({self._number_of_neighbors}) has to be less than or equal to"
                    f" the sample size ({training_set.number_of_rows})."
                ),
            )

(Same in the regression version.)

We can replace this with the new OutOfBoundsError, but then we cannot customize the error message as much. (It does allow for a name keyword argument, so we can tell the user the name of the number_of_neighbors parameter, but we cannot talk about the "sample size" anymore.)

We could make the OutOfBoundsError more flexible to also allow passing arbitrary strings to it (for example with a new custom_message: str keyword argument). But then the error itself gets somewhat complicated to use...

Let's keep this one as is. OutOfBoundsError makes sense if the bounds are some fixed constant rather than something like the length of a list.

🎉 This PR is included in version 0.15.0 🎉

The release is available on:

• v0.15.0
• GitHub release

Your semantic-release bot 📦🚀