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.

Sign up for GitHub

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

Jump to bottom

UserWarnings thrown in in test suite #399

Closed
zzril opened this issue Jun 27, 2023 · 4 comments · Fixed by #417
Closed

UserWarnings thrown in in test suite #399

zzril opened this issue Jun 27, 2023 · 4 comments · Fixed by #417
Assignees
@zzril
Labels
cleanup 🧹 Refactorings and other tasks that improve the code released Included in a release testing 🧪 Additional automated tests

Comments

@zzril
Copy link
Contributor

zzril commented Jun 27, 2023 •
edited
Loading

Is your feature request related to a problem?

3 UserWarnings from tests/safeds/data/tabular/transformation/test_imputer.py and .../_table_transformer.py and .../test_one_hot_encoder.py are currently reported as not being catched by our test suite.
(Tested on 54a6040.)

Desired solution

Find the cause of the warnings and handle them accordingly.

If they're meant to be thrown, catch them in the tests.

Ideally, the linter should also be updated not not even allow the tests to pass with these uncatched warnings thrown (see #357).
@zzril zzril added testing 🧪 Additional automated tests cleanup 🧹 Refactorings and other tasks that improve the code labels Jun 27, 2023
@github-project-automation github-project-automation bot moved this to Backlog in Library Jun 27, 2023
@lars-reimann
Copy link
Member

lars-reimann commented Jun 28, 2023 •
edited
Loading

Small clarification: The warnings should be caught in the implementation and not just in the tests.

This issue is about warnings in our code, so we should adjust the tests accordingly (after ensuring that the implementation is indeed correct).

@zzril zzril changed the title UserWarnings not catched in test suite UserWarnings thrown in in test suite Jun 28, 2023
@zzril
Copy link
Contributor Author

zzril commented Jun 28, 2023

Changed the description accordingly.

@daniaHu daniaHu moved this from Backlog to Todo in Library Jun 30, 2023
@zzril zzril self-assigned this Jul 4, 2023
@zzril zzril linked a pull request Jul 4, 2023 that will close this issue
test: Handle UserWarnings correctly in tests #417
Merged
@zzril zzril mentioned this issue Jul 4, 2023
Merged
@zzril
Copy link
Contributor Author

zzril commented Jul 4, 2023 •
edited
Loading

One of the warnings is the OneHotEncoder telling us that a column consisting of only nan values technically contains numerical data and thus does not really need to be one-hot-encoded.

I think the warning generally makes sense (although this case is somewhat pathologcal), so I would decide for just silencing it in the specific test that raised it.

The fact that the warning talks about "numerical" data (when in reality it's just nan values) might be confusing for some users, though.
Might want to think about detecting that specific case (all values in a colunm are nan) and raising a more specific / better-worded warning in that case.

@zzril zzril moved this from Todo to Ready for Review in Library Jul 4, 2023
@zzril zzril closed this as completed in #417 Jul 7, 2023
zzril added a commit that referenced this issue Jul 7, 2023
@zzril @megalinter-bot @Marsmaennchen221
test: Handle UserWarnings correctly in tests (#417)
388ab2d 
Closes #399.

### Summary of Changes

* Ignore the `UserWarning` about data being already numerical thrown by
`OneHotEncoder` in `nan` testcase.
* Ignore `UserWarning`s about multiple most common values existing
thrown by `Imputer` in testcases that are not meant to test for
warnings.

---------

Co-authored-by: megalinter-bot <[email protected]>
Co-authored-by: Alexander <[email protected]>
@github-project-automation github-project-automation bot moved this from Ready for Review to ✔️ Done in Library Jul 7, 2023
@lars-reimann
Copy link
Member

🎉 This issue has been resolved 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
Assignees

@zzril zzril

Labels
cleanup 🧹 Refactorings and other tasks that improve the code released Included in a release testing 🧪 Additional automated tests
Projects
Library
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: Handle UserWarnings correctly in tests Safe-DS/Library
2 participants
@lars-reimann @zzril