Threshold must be a real number #322

mvargas33 · 2021-09-14T12:24:06Z

Closes #219

As stated in the issue #219 , the threshold variable for _PairsClassifierMixin must be a real number (float or integer).

A type check has been added in set_threshold() method, following the same format/guideline of type checking in other parts of the code.

The threshold set from calibrate_threshold() method is guaranteed to be float, so no additional type checking is needed.

terrytangyuan

LGTM. Others PTAL

bellet

Thanks @mvargas33!
It would be nice to add a couple of tests to ensure we have the expected behavior

@terrytangyuan @perimosocordiae @wdevazelhes: I can't find the automatic CI / coverage reports - is it me or something is broken there?

terrytangyuan · 2021-09-14T14:17:31Z

From https://travis-ci.org/github/scikit-learn-contrib/metric-learn/builds, it says Since June 15th, 2021, the building on travis-ci.org is ceased. Please use travis-ci.com from now on. (Travis is no longer free)

We should probably switch to use GitHub Actions now. Many projects have already migrated.

bellet · 2021-09-14T14:37:33Z

From https://travis-ci.org/github/scikit-learn-contrib/metric-learn/builds, it says Since June 15th, 2021, the building on travis-ci.org is ceased. Please use travis-ci.com from now on. (Travis is no longer free)

We should probably switch to use GitHub Actions now. Many projects have already migrated.

Indeed it seems CI / codecov checks are no longer working (last ones are 4 months ago). Switching to GitHub Actions sounds good - do you happen to know how to go about it? I've never used it.

terrytangyuan · 2021-09-14T14:54:12Z

It's pretty straightforward to set up. Just create a .yml file in .github/workflows with the testing steps. A template can be found here.

terrytangyuan · 2021-09-14T14:55:45Z

Created #325 to track that separately. Feel free to pick it up.

perimosocordiae · 2021-09-15T19:52:40Z

metric_learn/base_metric.py

@@ -410,6 +410,10 @@ def set_threshold(self, threshold):
    """
    check_is_fitted(self, 'preprocessor_')

+    if threshold is None or not isinstance(threshold, (int, float)):


No need to check is None here, the isinstance check will handle None values as expected.

It may be even simpler to remove this checking and call self.threshold_ = float(threshold) instead. This will raise a ValueError if given invalid input, and also handles a broader range of float-like types.

I agree with this simpler solution

I agree as well. Perhaps writing a couple of test to make sure this raises the right error is still a good idea

Ok! Will do

…l case.

mvargas33 · 2021-09-21T14:09:12Z

I made a test to check correct behavior. The float(threshold) works for almost all datatypes, but not for boolean as float(True)=1.0, so I had to add isinstance() just to check the boolean type.

bellet · 2021-09-21T14:30:13Z

metric_learn/base_metric.py

+    if threshold is isinstance(threshold, (bool)):
+        raise ValueError('Parameter threshold must be a real number. '
+                         'Got {} instead.'.format(type(threshold)))
+    self.threshold_ = float(threshold)


Two comments:

Not sure if it makes sense to make an exception for bool

Maybe it is good to throw the custom error in all cases, which may be more informative for users? what do you think @perimosocordiae?

I agree, and the first approach was that, then I changed it to float(threshold). But given that it works for all except bool, maybe its better to use the custom error for all cases as proposed initially.

Sorry, this PR fell off my radar for a while. I tend to lean toward being permissive about these things, so I don't mind threshold=True resulting in a threshold of 1.0 here. If you do want to explicitly forbid it, though, that's fine by me. The check can be simpler:

if isinstance(threshold, bool): raise ...

I am fine with being permissive as well. My concern is whether the generic ValueError will be explicit enough for the user, or whether we should throw the custom error message in all cases.

metric_learn/base_metric.py

mvargas33

As of now the code is

  if threshold is isinstance(threshold, (bool)):
    raise ValueError('Parameter threshold must be a real number. '
                     'Got {} instead.'.format(type(threshold)))
  self.threshold_ = float(threshold)  # Will throw ValueError/TypeError

But if we would like to be permissive and display a custom message I would suggest the original solution:

  if not isinstance(threshold, (int, float)):
    raise ValueError('Parameter threshold must be a real number. '
                     'Got {} instead.'.format(type(threshold)))
  self.threshold_ = float(threshold)  # int -> float

It is used this way in _validate_calibration_params static method in PairClassifiers.

I've updated the test as well, now its parametrized.

metric_learn/base_metric.py

bellet

Some last nitpicks...

(If I'm not mistaken, the current solution is not permissive for bool: it would throw a ValueError, wouldn't it?)

bellet · 2021-10-28T15:56:35Z

metric_learn/base_metric.py

+    if not isinstance(threshold, (int, float)):
+      raise ValueError('Parameter threshold must be a real number. '
+                       'Got {} instead.'.format(type(threshold)))
+    self.threshold_ = float(threshold)  # int -> float


sorry to nitpick but can't you just call self.threshold_ = float(threshold) at the beginning and throw the custom warning if a ValueError is raised ? This would avoid calling isinstance; and would just let bool go through (converting to 0/1), which is fine.

I changed it for this

try: self.threshold_ = float(threshold) except Exception: raise ValueError('Parameter threshold must be a real number. ' 'Got {} instead.'.format(type(threshold)))

I guess if a warning is shown instead, the code can fail somewhere else, because it won't stop the execution.

And forgot to mention that isinstance(threshold, (int, float)) is permissive for bool. float(threshold) is also permissive.

test/test_pairs_classifiers.py

bellet

LGTM - will let @perimosocordiae check that all is good for him and merge

perimosocordiae · 2021-10-29T15:50:10Z

metric_learn/base_metric.py

-    self.threshold_ = threshold
+    try:
+      self.threshold_ = float(threshold)
+    except Exception:


except ValueError

(the bare Exception will catch too many unrelated errors)

I had to catch ValueError and TypeError. Most cases fall into TypeError, but String fall into ValueError with the funtion float():

ValueError("could not convert string to float: 'ABC'")

Considering both types of exceptions, the test passes.

perimosocordiae · 2021-11-02T16:03:20Z

Merged, thanks!

mvargas33 added 2 commits September 13, 2021 14:53

Add venv to gitignore

8fc975c

Check if threshold is a real value

40bbce8

terrytangyuan approved these changes Sep 14, 2021

View reviewed changes

bellet requested changes Sep 14, 2021

View reviewed changes

terrytangyuan mentioned this pull request Sep 14, 2021

Migrate CI to GitHub Actions #325

Closed

perimosocordiae requested changes Sep 15, 2021

View reviewed changes

mvargas33 and others added 4 commits September 17, 2021 10:21

Simplified threshold type-check

60dc0d4

Follow linter rules

a92d2c9

Fix last linter error

ddce7db

Add test to check correct behaviour. Sacrified simplicity for the boo…

ac4584e

…l case.

bellet reviewed Sep 21, 2021

View reviewed changes

mvargas33 added 2 commits October 28, 2021 11:04

Merge remote-tracking branch 'upstream/master' into feat-threshold

0d80fdf

Update test. Stick to custom message. It's bool permissive

166228f

mvargas33 commented Oct 28, 2021

View reviewed changes

metric_learn/base_metric.py Outdated Show resolved Hide resolved

mvargas33 requested review from terrytangyuan, bellet and perimosocordiae October 28, 2021 09:45

bellet reviewed Oct 28, 2021

View reviewed changes

mvargas33 added 2 commits October 28, 2021 18:10

Explicit boolean permissive case in test

47c21ac

Changed isinstance for custom ValueError message

81c7f73

mvargas33 requested a review from bellet October 29, 2021 08:27

bellet approved these changes Oct 29, 2021

View reviewed changes

perimosocordiae approved these changes Oct 29, 2021

View reviewed changes

TypeError for most input. ValueError for String case.

796ba93

mvargas33 requested a review from perimosocordiae November 2, 2021 13:10

perimosocordiae approved these changes Nov 2, 2021

View reviewed changes

perimosocordiae merged commit 964f28d into scikit-learn-contrib:master Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Threshold must be a real number #322

Threshold must be a real number #322

mvargas33 commented Sep 14, 2021 •

edited by bellet

Loading

terrytangyuan left a comment

bellet left a comment

terrytangyuan commented Sep 14, 2021 •

edited

Loading

bellet commented Sep 14, 2021

terrytangyuan commented Sep 14, 2021

terrytangyuan commented Sep 14, 2021

perimosocordiae Sep 15, 2021

mvargas33 Sep 17, 2021

bellet Sep 17, 2021

mvargas33 Sep 17, 2021

mvargas33 commented Sep 21, 2021

bellet Sep 21, 2021

mvargas33 Sep 21, 2021 •

edited

Loading

perimosocordiae Oct 27, 2021

bellet Oct 27, 2021

mvargas33 left a comment •

edited

Loading

bellet left a comment •

edited

Loading

bellet Oct 28, 2021

mvargas33 Oct 28, 2021 •

edited

Loading

mvargas33 Oct 28, 2021

bellet left a comment •

edited

Loading

perimosocordiae Oct 29, 2021

mvargas33 Nov 2, 2021

perimosocordiae commented Nov 2, 2021

Threshold must be a real number #322

Threshold must be a real number #322

Conversation

mvargas33 commented Sep 14, 2021 • edited by bellet Loading

terrytangyuan left a comment

Choose a reason for hiding this comment

bellet left a comment

Choose a reason for hiding this comment

terrytangyuan commented Sep 14, 2021 • edited Loading

bellet commented Sep 14, 2021

terrytangyuan commented Sep 14, 2021

terrytangyuan commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvargas33 commented Sep 21, 2021

Choose a reason for hiding this comment

mvargas33 Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvargas33 left a comment • edited Loading

Choose a reason for hiding this comment

bellet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvargas33 Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perimosocordiae commented Nov 2, 2021

mvargas33 commented Sep 14, 2021 •

edited by bellet

Loading

terrytangyuan commented Sep 14, 2021 •

edited

Loading

mvargas33 Sep 21, 2021 •

edited

Loading

mvargas33 left a comment •

edited

Loading

bellet left a comment •

edited

Loading

mvargas33 Oct 28, 2021 •

edited

Loading

bellet left a comment •

edited

Loading