Skip to content

MAINT Parameters validation for graph.single_source_shortest_path_length #26091

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

Conversation

ChVeen
Copy link
Contributor

@ChVeen ChVeen commented Apr 4, 2023

Reference Issues/PRs

Towards #24862

What does this implement/fix? Explain your changes.

This PR implements automatic parameters validation for sklearn.utils.graph.single_source_shortest_path_length

Any other comments?

@ChVeen ChVeen force-pushed the Parameters-validation-for-single_source_shortest_path_length branch from ff82d03 to 16b2171 Compare April 4, 2023 22:57
@jeremiedbb jeremiedbb added No Changelog Needed Validation related to input validation labels Apr 5, 2023
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ChVeen. Here are some comments

@ChVeen ChVeen force-pushed the Parameters-validation-for-single_source_shortest_path_length branch from 16b2171 to 448c7b1 Compare April 5, 2023 19:39
@ChVeen
Copy link
Contributor Author

ChVeen commented Apr 5, 2023

@jeremiedbb, thanks for all the hints, I solved the issues.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ChVeen

@ChVeen ChVeen force-pushed the Parameters-validation-for-single_source_shortest_path_length branch from 036fc09 to b358b7d Compare April 12, 2023 20:47
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM. Running the CI to make sure the error was just transient.

@glemaitre
Copy link
Member

I am going to try updating the minimal version of pytest. We are 2 major versions of difference which is quite a lot.

@glemaitre glemaitre self-requested a review April 14, 2023 15:58
@glemaitre
Copy link
Member

OK, this is interesting. It seems that the INTERNALERROR is not anymore.

@ChVeen
Copy link
Contributor Author

ChVeen commented Apr 15, 2023

I found out that the files

  • build_tools/azure/debian_atlas_32bit_lock.txt
  • build_tools/azure/debian_atlas_32bit_requirements.txt

were not updated to the new pytest version.
So I tried this out by updating them manually.

Now, the INTERNALERROR seems to be back... 🤷
Possibly one has to call the update_environments_and_lock_files.py script in the build_tools folder.
Unfortunately, this doesn't work in my codespace.
I could not figure out why (up to now).
Maybe it is a good idea that someone else should call this script instead to be sure to update the above files in a better way...

@glemaitre
Copy link
Member

It is what I intended to do here: #26184

…test_path_length

The new pytest version is needed as committed to main by PR scikit-learn#26184
Signed-off-by: Christian Veenhuis <124370897+ChVeen@users.noreply.github.com>
@ChVeen
Copy link
Contributor Author

ChVeen commented Jun 4, 2023

@glemaitre Finally it works. It seems that pytest did not like the . operator for referring to modules in the same folder. Thus, I replaced

from ._param_validation import Interval, Integral, validate_params

by

from ..utils._param_validation import Interval, Integral, validate_params.

@ChVeen ChVeen requested a review from jeremiedbb June 4, 2023 16:25
@github-actions
Copy link

github-actions bot commented Jun 27, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: fac27b5. Link to the linter CI: here

@jeremiedbb jeremiedbb enabled auto-merge (squash) June 27, 2023 14:11
@jeremiedbb jeremiedbb merged commit e3590c8 into scikit-learn:main Jun 27, 2023
@ChVeen ChVeen deleted the Parameters-validation-for-single_source_shortest_path_length branch June 30, 2023 19:16
punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023
…gth (scikit-learn#26091)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
…gth (scikit-learn#26091)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants