TST Work-around for Ubuntu atlas float32 failure #24198

lesteve · 2022-08-18T10:22:59Z

This fixes the Ubuntu atlas issue seen in #24131 (comment). I have tried using the magic [all random seeds] syntax for the CI, let's see if that works 🤞.

This is definitely a kludgy work-around but maybe good enough since the failure seems to happen quite rarely (see below for more details)?

The snippet below reproduces the problem (both for Ubuntu 20.04 with atlas and when installing from conda-forge). Trying different random_state for the fastica call, the exception happens 1-3 times out of 10000 so maybe this is rare enough to use this work-around.

import numpy as np
from scipy import stats
from sklearn.decomposition import fastica
import sys

global_random_seed = 20
global_dtype = np.float32


def center_and_norm(x, axis=-1):
    x = np.rollaxis(x, axis)
    x -= x.mean(axis=0)
    x /= x.std(axis=0)


rng = np.random.RandomState(global_random_seed)
n_samples = 1000
# Generate two sources:
s1 = (2 * np.sin(np.linspace(0, 100, n_samples)) > 0) - 1
s2 = stats.t.rvs(1, size=n_samples, random_state=global_random_seed)
s = np.c_[s1, s2].T
center_and_norm(s)
s = s.astype(global_dtype)
s1, s2 = s

# Mixing angle
phi = 0.6
mixing = np.array([[np.cos(phi), np.sin(phi)], [np.sin(phi), -np.cos(phi)]])
mixing = mixing.astype(global_dtype)
m = np.dot(mixing, s)
center_and_norm(m)

algo = 'deflation'
nl = 'logcosh'
whiten = 'arbitrary-variance'

problematic_random_state = 13441

k_, mixing_, s_ = fastica(
    m.T, fun=nl, whiten=whiten, algorithm=algo, random_state=problematic_random_state
)

More than happy to create an issue about the division by zero in fastica for a more proper fix

lesteve · 2022-08-18T13:55:27Z

In the last commit, fast_ica_simple pass for all random seeds in the Ubuntu atlas build, which is what I was trying to make sure of.

I can reproduce locally the Debian 32 bit failure on main. I don't think we have seen the failure on Debian 32 bit yet in the nightly build. This is slightly weird since this fails for the vast majority (~70%) of global random seeds locally, so I would have expected the nightly build to have picked this up with its random global seed set by SKLEARN_TESTS_GLOBAL_RANDOM_SEED="any" ...

lesteve · 2022-08-18T14:47:25Z

In the last commit the build pass for all the seeds in the Debian 32bit build

lesteve · 2022-08-18T15:49:54Z

I opened #24202 to be able to run the float32 tests using [float32 test] in the commit message. #24202 should be merged first so I can reuse it here.

ogrisel

~~It would be good to extract a minimal reproducer to report the bug upstream~~ but in the mean time, +1 for this workaround.

Actually I re-read the linked comment and there is probably no bug to report to SciPy / LAPACK. Maybe our FastICA code would benefit from a special if condition to avoid dividing by zero?

ogrisel · 2022-08-23T08:54:33Z

sklearn/decomposition/tests/test_fastica.py

+    # XXX we generate noise even when add_noise=False to avoid a rare case
+    # where the test fails for float32. For more details see:
+    # https://github.com/scikit-learn/scikit-learn/issues/24131#issuecomment-1208091119


Suggested change

# XXX we generate noise even when add_noise=False to avoid a rare case

# where the test fails for float32. For more details see:

# https://github.com/scikit-learn/scikit-learn/issues/24131#issuecomment-1208091119

# XXX we consume the RNG even when add_noise=False to avoid a rare case

# where the test fails for float32 with Atlas. For more details see:

# https://github.com/scikit-learn/scikit-learn/issues/24131#issuecomment-1208091119

ogrisel

Changing my review state because we should probably attempt to fix the division by zero instead of using a workaround in the tests.

lesteve · 2022-08-24T09:20:55Z

I opened #24244 to fix the debian 32 bit build (adding atol) to fix #24240. I'll tackle the more controversial fastica division by zero later.

jeremiedbb · 2022-09-20T16:38:03Z

We already observed numerical instabilities when reviewing #22806
The following was added to try to mitigate them. Looks like it was not good enough, or does it come from another part of the code ?

def _sym_decorrelation(W):
    """Symmetric decorrelation
    i.e. W <- (W * W.T) ^{-1/2} * W
    """
    s, u = linalg.eigh(np.dot(W, W.T))
    # Avoid sqrt of negative values because of rounding errors. Note that   #
    # np.sqrt(tiny) is larger than tiny and therefore this clipping also    #
    # prevents division by zero in the next step.                           #
    s = np.clip(s, a_min=np.finfo(W.dtype).tiny, a_max=None)                #  <--

    # u (resp. s) contains the eigenvectors (resp. square roots of
    # the eigenvalues) of W * W.T
    return np.linalg.multi_dot([u * (1.0 / np.sqrt(s)), u.T, W])

There's also this old issue that already mentions the instabilities in fastica #2735 and it looks to be the same division by 0 issue from _sym_decorrelation. There's even a PR to propose a possibly more stable way to find the eigen values, using a svd (at the possible cost of a slowdown): #2738. Maybe it's worth looking at that PR

ogrisel · 2022-10-14T08:10:37Z

For reference, I tried to run SKLEARN_RUN_FLOAT32_TESTS=1 SKLEARN_TESTS_GLOBAL_RANDOM_SEED='all' pytest sklearn/decomposition/tests/test_fastica.py -k test_fastica_simple -k False -v on the current main on macos/arm64 with OpenBLAS from conda-forge and this test is stable for all the seeds.

It could be a numerical stability issue in Atlas itself. Still it would be interesting to check if a revived SVD solver as in #2738 makes this test stable on Atlas.

jjerphan · 2022-11-08T10:42:19Z

Does this PR completes or adapts #24244 to resolve ~~#24198~~ #24131 (comment)?

lesteve · 2022-11-08T11:13:50Z

#24244 was an easy fix to fix some of the failures by using a higher tolerance for float32 tests.

This PR was an hacky and questionable attempt to hide #24131 (comment) under the carpet. To be honest if we want a quick fix to not see the CI red from time to time, we can always skip the test for Atlas and the single global random seed for which it fails.

Looks like the consensus instead is to try to fix the fastica algorithm. Full disclosure: I am not planning to work on this in the near future.

jjerphan · 2022-11-10T07:51:36Z

Thanks for providing context.

I think xfailing for this configuration in the meantime is slightly preferable over changing the tolerance. Do you think we can proceed as such?

lesteve · 2022-11-10T10:13:42Z

We could very likely pytest.xfail for the atlas configuration and the particular failing seed.

From what I could quickly gather, it does not seem like there is a reliable way of using numpy.show_config() to figure out whether atlas is used so the easiest work-around would be to use the DISTRIB environment variable ...

jjerphan · 2022-11-10T12:29:17Z

Do you think threadpoolctl.threadpool_info would allow asserting whether ATLAS is used?

lesteve · 2022-11-10T13:06:21Z

I think threadpoolctl_info is wrong here too (I am going to guess that this is a edge case known by threadpoolctl developers)

threadpoolctl_info says openblas:

>>> import numpy as np
>>> threadpoolctl.threadpool_info()
[{'user_api': 'blas', 'internal_api': 'openblas', 'prefix': 'libopenblas', 'filepath': '/usr/lib/x86_64-linux-gnu/openblas-pthread/libopenblasp-r0.3.20.so', 'version': '0.3.20', 'threading_layer': 'pthreads', 'architecture': 'Haswell', 'num_threads': 4}]

But using ldd I get atlas:

❯ ldd /usr/lib/python3/dist-packages/numpy/core/_multiarray_umath.cpython-310-x86_64-linux-gnu.so | grep blas
	libblas.so.3 => /lib/x86_64-linux-gnu/libblas.so.3 (0x00007fe1c29bd000)```

❯ readlink -e /lib/x86_64-linux-gnu/libblas.so.3
/usr/lib/x86_64-linux-gnu/atlas/libblas.so.3.10.3

jeremiedbb · 2022-11-10T15:20:53Z

Do you think threadpoolctl.threadpool_info would allow asserting whether ATLAS is used?

ATLAS is not supported by threadpoolctl because it doesn't use multithreading.

I think threadpoolctl_info is wrong here too

Can you post the full result of ldd ? and ideally resolve the symlinks ?

lesteve · 2022-11-10T16:22:59Z

❯ ldd /usr/lib/python3/dist-packages/numpy/core/_multiarray_umath.cpython-310-x86_64-linux-gnu.so            
	linux-vdso.so.1 (0x00007ffc3f6c8000)
	libgtk3-nocsd.so.0 => /lib/x86_64-linux-gnu/libgtk3-nocsd.so.0 (0x00007fd1c4e00000)
	libblas.so.3 => /lib/x86_64-linux-gnu/libblas.so.3 (0x00007fd1c4dbd000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fd1c4cd6000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fd1c4aae000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fd1c540c000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fd1c5016000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fd1c500f000)
	libatlas.so.3 => /lib/x86_64-linux-gnu/libatlas.so.3 (0x00007fd1c4745000)
	libgfortran.so.5 => /lib/x86_64-linux-gnu/libgfortran.so.5 (0x00007fd1c4469000)
	libquadmath.so.0 => /lib/x86_64-linux-gnu/libquadmath.so.0 (0x00007fd1c4421000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fd1c4401000)

test_fastica_simple

jeremiedbb · 2022-11-10T17:13:42Z

hum, maybe libopenblas is linked to another .so because threadpoolctl only looks for libopenblas being in the name of a resolved symlink, meaning that it found one

lesteve · 2022-11-10T17:26:00Z

The work-around works locally (when setting DISTRIB=ubuntu), it seems like it works in the CI too, e.g. this Ubuntu Atlas build shows a x in the middle of the points which is the xfailed build.

jjerphan

LGTM.

sklearn/decomposition/tests/test_fastica.py

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

jeremiedbb

I'm okay with that until someone is motivated to take a look at fastica :)

lesteve · 2022-11-25T09:53:39Z

Thanks I'll create an issue about it when I get the time.

lesteve · 2022-11-25T10:06:19Z

Done in #25038

github-actions bot added the module:decomposition label Aug 18, 2022

lesteve added the No Changelog Needed label Aug 18, 2022

This was referenced Aug 18, 2022

CI Use "any" global random seed and run float32 tests in Azure docker-based builds #24201

Closed

CI add ability to run float32 tests by commit message #24202

Merged

ogrisel approved these changes Aug 23, 2022

View reviewed changes

ogrisel added the Quick Review For PRs that are quick to review label Aug 23, 2022

ogrisel requested changes Aug 23, 2022

View reviewed changes

lesteve removed the Quick Review For PRs that are quick to review label Aug 24, 2022

work-around for float32 tests [all random seeds]

73a418b

test_fastica_simple

lesteve force-pushed the fix-fastica-float32 branch from 7d2ede5 to 73a418b Compare November 10, 2022 16:50

lesteve added 2 commits November 10, 2022 17:58

work-around for float32 tests [float32] [all random seeds]

c478256

test_fastica_simple

work-around for float32 tests [float32] [all random seeds]

b4ddf1d

test_fastica_simple

jjerphan approved these changes Nov 14, 2022

View reviewed changes

sklearn/decomposition/tests/test_fastica.py Outdated Show resolved Hide resolved

Update sklearn/decomposition/tests/test_fastica.py

40bbb22

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

lesteve added the Quick Review For PRs that are quick to review label Nov 18, 2022

jeremiedbb approved these changes Nov 25, 2022

View reviewed changes

jeremiedbb merged commit 40c0f7d into scikit-learn:main Nov 25, 2022

lesteve deleted the fix-fastica-float32 branch November 25, 2022 09:53

This was referenced Nov 25, 2022

⚠️ CI failed on Linux.ubuntu_atlas ⚠️ #24131

Closed

Instability in fastica for float32 #25038

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST Work-around for Ubuntu atlas float32 failure #24198

TST Work-around for Ubuntu atlas float32 failure #24198

TST Work-around for Ubuntu atlas float32 failure #24198

TST Work-around for Ubuntu atlas float32 failure #24198

Conversation

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

Choose a reason for hiding this comment