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

add validation on _window_size param in topo extractor #1284

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

Lopa10ko
Copy link
Collaborator

@Lopa10ko Lopa10ko commented Apr 14, 2024

This is a 🐛 bug fix.

Summary

  • add mandatory non-negative and non-zero validation on window size param inside a TopologicalFeaturesImplementation fit (1c04aad)
  • add unit test for a non-negative window check and extraction method validation (77a9c78)

Context

fixed integration test failure with: ValueError: Found array with 0 feature(s) (shape=(3, 0)) while a minimum of 1 is required by check_pairwise_arrays.

@Lopa10ko Lopa10ko self-assigned this Apr 14, 2024
Copy link

docu-mentor bot commented Apr 14, 2024

👋 Hi, I'm @docu-mentor, an LLM-powered GitHub app
powered by Anyscale Endpoints
that gives you actionable feedback on your writing.

Simply create a new comment in this PR that says:

@docu-mentor run

and I will start my analysis. I only look at what you changed
in this PR. If you only want me to look at specific files or folders,
you can specify them like this:

@docu-mentor run doc/ README.md

In this example, I'll have a look at all files contained in the "doc/"
folder and the file "README.md". All good? Let's get started!

@Lopa10ko Lopa10ko added bug Something isn't working test The additon or modification of the unit test labels Apr 14, 2024
Copy link
Contributor

github-actions bot commented Apr 14, 2024

All PEP8 errors has been fixed, thanks ❤️

Comment last updated at

@nicl-nno
Copy link
Collaborator

А можно это модульным тестом покрыть для надежности?

@Lopa10ko
Copy link
Collaborator Author

Lopa10ko commented Apr 14, 2024

А можно это модульным тестом покрыть для надежности?

да, можно и нужно - сделаю. (77a9c78)

Note

в другом PR планируется поработать в целом над этим тестом, потому что в последнее время только он и падает из интеграционников: по крайней мере прокидывание NaN или float.inf в оптимизатор явно не является ожидаемым - это вновь проблемы ETSModel #1285

@Lopa10ko Lopa10ko changed the title fix test_api_ts_forecasting_example fix test_api_ts_forecasting_example by validating _window_size param Apr 18, 2024
@Lopa10ko Lopa10ko changed the title fix test_api_ts_forecasting_example by validating _window_size param add validation on _window_size param in topo extractor Apr 18, 2024
@Lopa10ko Lopa10ko merged commit 79ec471 into master Apr 22, 2024
9 checks passed
@Lopa10ko Lopa10ko deleted the fix-window-no-negative branch June 5, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test The additon or modification of the unit test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants