Skip to content

FIX change the meaning of include_boundaries in check_scalar #20921

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
Merged
merged 6 commits into from
Sep 7, 2021
Merged

FIX change the meaning of include_boundaries in check_scalar #20921

merged 6 commits into from
Sep 7, 2021

Conversation

glemaitre
Copy link
Member

I think that the current definition of the parameter closed in check_scalar is counterintuitive.
Indeed, min_val and max_val define the interval of proper value and closed will define whether or not to include these values. Thus left should include min_val in the interval, right should include max_val in the range of correct values.

@glemaitre
Copy link
Member Author

ping @adrinjalali @ogrisel

@jeremiedbb
Copy link
Member

Maybe change the name to include_bounds if it's not intuitive. It's not an interval object after all so it might make more sense.

@adrinjalali
Copy link
Member

I find closed the best word and the most mathematically sane way of saying it, but if more people would be happy with it, I could be happy with include_bounds.

@ogrisel
Copy link
Member

ogrisel commented Sep 3, 2021

I think include_bounds or maybe the more correct include_boundaries would be more explicit for users with a lighter background in math.

+1 for include_boundaries.

jeremiedbb
jeremiedbb reviewed Sep 3, 2021
View reviewed changes
sklearn/cluster/_affinity_propagation.py
@@ -469,7 +469,7 @@ def fit(self, X, y=None):
target_type=numbers.Real,
min_val=0.5,
max_val=1,
closed="right",
Copy link
Member

@jeremiedbb jeremiedbb Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

the description of the param says "between 0.5 and 1". If 1 is excluded, maybe we should mention it there

Copy link
Member Author

Choose a reason for hiding this comment

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

let me second check the previous code to be sure but I think it was excluded.

Copy link
Member Author

@glemaitre glemaitre Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

my bad the interval is 0.5 < dumping <= 1.0 no I read it bad again, it is 0.5 <= dumping < 1.0

Copy link
Member

Choose a reason for hiding this comment

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

hum, but 0.5 is the default so it should be included :)

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI: https://github.com/scikit-learn/scikit-learn/pull/20723/files#diff-62083de22888eadb572404f8f7255a19a74370eeaf2a893858b066d90ada979eL135-L136

Copy link
Member Author

Choose a reason for hiding this comment

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

hum, but 0.5 is the default so it should be included :)

Yes, I edited my answer. I confused the valid interval and the condition used to raise the error :)

@glemaitre glemaitre changed the title FIX change the meaning of closed in check_scalar FIX change the meaning of include_boundaries in check_scalar Sep 3, 2021
@glemaitre glemaitre mentioned this pull request Sep 3, 2021
Merged
3 tasks
@glemaitre
Copy link
Member Author

gentle ping @ogrisel @jeremiedbb :)

jeremiedbb
jeremiedbb approved these changes Sep 6, 2021
View reviewed changes
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, just minor comments. Thanks @glemaitre !

@glemaitre @jeremiedbb
Apply suggestions from code review
51266de 
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@glemaitre glemaitre added this to the 1.0 milestone Sep 7, 2021
@glemaitre
Copy link
Member Author

I would like to have this one for 1.0 to not release check_scalar with a bug :) ping @adrinjalali

@adrinjalali
Copy link
Member

tagging #20965

ogrisel
ogrisel approved these changes Sep 7, 2021
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM. I did some manual check to be sure because it's hard to reason with double negations in the code or the heavy use of pytest.parametrize but it works as it's supposed to.

Let's just wait for the CI to pass with the new test case I just pushed.

@ogrisel ogrisel merged commit 8bf3aa5 into scikit-learn:main Sep 7, 2021
@ogrisel ogrisel added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Sep 7, 2021
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Sep 7, 2021
@glemaitre @jeremiedbb
@ogrisel @thomasjpfan
FIX change the meaning of include_boundaries in check_scalar (scikit-…
686b3cc 
…learn#20921)

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
adrinjalali pushed a commit to adrinjalali/scikit-learn that referenced this pull request Sep 10, 2021
@glemaitre @jeremiedbb
@ogrisel @adrinjalali
FIX change the meaning of include_boundaries in check_scalar (scikit-…
82c280a 
…learn#20921)

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
adrinjalali pushed a commit that referenced this pull request Sep 14, 2021
@glemaitre @jeremiedbb
@ogrisel @adrinjalali
FIX change the meaning of include_boundaries in check_scalar (#20921)
d39c50f 
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@glemaitre @jeremiedbb
@ogrisel @samronsin
FIX change the meaning of include_boundaries in check_scalar (scikit-…
87346cf 
…learn#20921)

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:cluster module:utils No Changelog Needed To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@glemaitre @jeremiedbb @adrinjalali @ogrisel