scikit-learn-contrib / metric-learn

Metric learning algorithms in Python
http://contrib.scikit-learn.org/metric-learn/
MIT License
1.4k stars 234 forks source link

Fix 7 sources of warnings in the tests #339

Closed mvargas33 closed 3 years ago

mvargas33 commented 3 years ago
  1. First warning

    /metric_learn/scml.py:618: DeprecationWarning: `np.int` is a deprecated alias for the builtin `int`. To silence this warning, use `int` by itself. Doing this will not modify any behavior and is safe. When replacing `np.int`, you may wish to use e.g. `np.int64` or `np.int32` to specify the precision. If you wish to review your current use, check the release note link for additional information.
    Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
    idx_set = [np.zeros((n_clusters, sum(k_class[0, :])), dtype=np.int),

    Solution: Replace np.int for np.int64 for more precision

  2. Second warning

    /metric_learn/itml.py:35: FutureWarning: arrays to stack must be passed as a "sequence" type such as list or tuple. Support for non-sequence iterables such as generators is deprecated as of NumPy 1.16 and will raise an error in the future.
    X = np.vstack({tuple(row) for row in pairs.reshape(-1, pairs.shape[2])})

    Solution: Do a refactor to get the same result. The above is equivalent to:

X = np.unique(np.vstack(input), axis=0)

Made a little script to verify the behaviour. Also, all tests pass.

  1. Third warning
    
    /metric-learn/test/test_utils.py:1170: PytestUnknownMarkWarning: Unknown pytest.mark.integration - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/mark.html
    @pytest.mark.integration

/metric-learn/test/test_utils.py:1070: PytestUnknownMarkWarning: Unknown pytest.mark.unit - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/mark.html @pytest.mark.unit

**Solution**: Register these marks in a pytest.ini file, as the [docs](https://doc.pytest.org/en/latest/example/markers.html#registering-markers) suggest

4. Fourth warning

/metric-learn/venv/lib/python3.8/site-packages/sklearn/utils/deprecation.py:87: FutureWarning: Function assert_warns_message is deprecated; assert_warns_message is deprecated in 1.0 and will be removed in 1.2.Use pytest.warns instead. warnings.warn(msg, category=FutureWarning)

**Solution**: Refactor the test to use pytest.warns isntead of old API

5. Fifth warning

/metric-learn/metric_learn/rca.py:115: FutureWarning: rcond parameter will change to the default of machine precision times max(M, N) where M and N are the input matrix dimensions. To use the future default and silence this warning we advise to pass rcond=None, to keep using the old, explicitly pass rcond=-1. tmp = np.linalg.lstsq(total_cov, inner_cov)[0]

**Solution**:" Change to future default, changing rcond=None now
6. Sixth warning

All user warnings that were not being catched in `test_generate_knntriplets` at `test_constraints.py`, and this is exactly was this test is meant to test.

**Solution**: Catch the `UserWarnings` with pytest.warns(UserWarning)

7. Seventh warning

By default any usage of SCML with the default constructor, will throw the following warning:

/metric_learn/scml.py:235: UserWarning: As no value for n_basis was selected, the number of basis will be set to n_basis= 320 warnings.warn('As no value for n_basis was selected, the number of '


If this warning will be always be thrown for `SCML` and `SCML_Sueprvized`, it should be catched with `pytest.warns(UserWarning)` and check that the message is exactly that "the value of n_basis was not selected".

Changed in `test_triplets_classifiers.py` as an example, if needed I can extend this pattern to many warnings of this kind thrown in `test_mahalanobis_mixin.py` and in other files.

**Overall result**: Reduced from 1.117 to 482 warnings across all tests. Many left are being thrown because of the last warning described
mvargas33 commented 3 years ago

Besides my own TOC to remove warnings, this is related to #189

perimosocordiae commented 3 years ago

Merged, thanks!

bellet commented 3 years ago

@mvargas33 as we discussed informally, for the last category of warnings related to SCML, I would rather provide the n_basis argument explicitly in all tests rather than adding a lot of lines of code to catch the warning like you have illustrated in test_triplets_classifiers.py. If you have some time to address this, you could open a separate PR for this

mvargas33 commented 3 years ago

@mvargas33 as we discussed informally, for the last category of warnings related to SCML, I would rather provide the n_basis argument explicitly in all tests rather than adding a lot of lines of code to catch the warning like you have illustrated in test_triplets_classifiers.py. If you have some time to address this, you could open a separate PR for this

Wiil do, in a separate PR. I agree this try/catch is an overkill and makes the code more illegible.