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

Fix 7 sources of warnings in the tests #339

Merged
merged 5 commits into from
Oct 21, 2021

Conversation

mvargas33
Copy link
Contributor

  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

  1. 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 suggest

  1. 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

  1. 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)

  1. 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
Copy link
Contributor Author

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

@@ -704,7 +704,7 @@ def _initialize_metric_mahalanobis(input, init='identity', random_state=None,
elif init == 'covariance':
if input.ndim == 3:
# if the input are tuples, we need to form an X by deduplication
X = np.vstack({tuple(row) for row in input.reshape(-1, n_features)})
X = np.unique(np.vstack(input), axis=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be faster in some cases, and more RAM-hungry in others. Overall I think it's probably fine, and if users are particularly sensitive to the RAM requirement they can perform their own dedup and pass a 2-d input directly.

@perimosocordiae perimosocordiae merged commit 6a4aaea into scikit-learn-contrib:master Oct 21, 2021
@perimosocordiae
Copy link
Contributor

Merged, thanks!

@bellet
Copy link
Member

bellet commented Oct 27, 2021

@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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants