Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: new error class OutOfBoundsError #438

Merged
merged 87 commits into from
Jul 11, 2023

Conversation

zzril
Copy link
Contributor

@zzril zzril commented Jul 7, 2023

Closes #262.

Summary of Changes

  • Introduced a new generic error class OutOfBoundsError that can be used to signal that a value is outside its expected range.
  • Updated Image to use the new error.
  • Updated Discretizer to use the new error.
  • Updated ML models with hyperparameters to use the new error.

@zzril zzril linked an issue Jul 7, 2023 that may be closed by this pull request
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 34 0 0 1.53s
✅ PYTHON mypy 34 0 2.97s
✅ PYTHON ruff 34 0 0 0.14s
✅ REPOSITORY git_diff yes no 0.1s

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 OX Security

@zzril zzril changed the title 262 custom error if number is out of bounds feat: new error class OutOfBoundsError Jul 7, 2023
@codecov
Copy link

codecov bot commented Jul 7, 2023

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     
Impacted Files Coverage Δ
src/safeds/data/image/containers/_image.py 100.00% <100.00%> (ø)
...safeds/data/tabular/transformation/_discretizer.py 100.00% <100.00%> (ø)
src/safeds/exceptions/__init__.py 100.00% <100.00%> (ø)
src/safeds/exceptions/_generic.py 100.00% <100.00%> (ø)
...c/safeds/ml/classical/classification/_ada_boost.py 100.00% <100.00%> (ø)
.../ml/classical/classification/_gradient_boosting.py 100.00% <100.00%> (ø)
...l/classical/classification/_k_nearest_neighbors.py 100.00% <100.00%> (ø)
...feds/ml/classical/classification/_random_forest.py 100.00% <100.00%> (ø)
...lassical/classification/_support_vector_machine.py 100.00% <100.00%> (ø)
src/safeds/ml/classical/regression/_ada_boost.py 100.00% <100.00%> (ø)
... and 7 more

@zzril
Copy link
Contributor Author

zzril commented Jul 8, 2023

@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.

@zzril
Copy link
Contributor Author

zzril commented Jul 10, 2023

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...

@zzril
Copy link
Contributor Author

zzril commented Jul 10, 2023

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.

@zzril zzril requested a review from lars-reimann July 10, 2023 23:17
@zzril zzril marked this pull request as ready for review July 10, 2023 23:18
@lars-reimann
Copy link
Member

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.

Copy link
Member

@lars-reimann lars-reimann left a comment

Choose a reason for hiding this comment

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

Thanks a lot, very nice work.

@lars-reimann lars-reimann merged commit 1f37e4a into main Jul 11, 2023
@lars-reimann lars-reimann deleted the 262-custom-error-if-number-is-out-of-bounds branch July 11, 2023 09:25
lars-reimann pushed a commit that referenced this pull request Jul 13, 2023
## [0.15.0](v0.14.0...v0.15.0) (2023-07-13)

### Features

* Add copy method for tables ([#405](#405)) ([72e87f0](72e87f0)), closes [#275](#275)
* add gaussian noise to image ([#430](#430)) ([925a505](925a505)), closes [#381](#381)
* add schema conversions when adding new rows to a table and schema conversion when creating a new table ([#432](#432)) ([6e9ff69](6e9ff69)), closes [#404](#404) [#322](#322) [#127](#127) [#322](#322) [#127](#127)
* add test for empty tables for the method `Table.sort_rows` ([#431](#431)) ([f94b768](f94b768)), closes [#402](#402)
* added color adjustment feature ([#409](#409)) ([2cbee36](2cbee36)), closes [#380](#380)
* added test_repr table tests ([#410](#410)) ([cb77790](cb77790)), closes [#349](#349)
* discretize table ([#327](#327)) ([5e3da8d](5e3da8d)), closes [#143](#143)
* Improve error handling of TaggedTable ([#450](#450)) ([c5da544](c5da544)), closes [#150](#150)
* Maintain tagging in methods inherited from `Table` class ([#332](#332)) ([bc73a6c](bc73a6c)), closes [#58](#58)
* new error class `OutOfBoundsError` ([#438](#438)) ([1f37e4a](1f37e4a)), closes [#262](#262)
* rename several `Table` methods for consistency ([#445](#445)) ([9954986](9954986)), closes [#439](#439)
* suggest similar columns if column gets accessed that doesnt exist ([#385](#385)) ([6a097a4](6a097a4)), closes [#203](#203)

### Bug Fixes

* added the missing ids in parameterized tests ([#412](#412)) ([dab6419](dab6419)), closes [#362](#362)
* don't warn if `Imputer` transforms column without missing values ([#448](#448)) ([f0cb6a5](f0cb6a5))
* Warnings raised by underlying seaborn and numpy libraries  ([#425](#425)) ([c4143af](c4143af)), closes [#357](#357)
@lars-reimann
Copy link
Member

🎉 This PR is included in version 0.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lars-reimann lars-reimann added the released Included in a release label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Included in a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom error if number is out of bounds
4 participants