Upgrade MXNet backend to MXNet.np interface by aarmey · Pull Request #225 · tensorly/tensorly

aarmey · 2021-02-06T17:13:19Z

This also removes the need for some of the MXNet-specific workarounds.

codecov · 2021-02-06T17:14:25Z

Codecov Report

Merging #225 (aa32605) into main (f0b701e) will increase coverage by 0.69%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
+ Coverage   86.35%   87.05%   +0.69%     
==========================================
  Files          82       82              
  Lines        3855     3846       -9     
==========================================
+ Hits         3329     3348      +19     
+ Misses        526      498      -28

Impacted Files	Coverage Δ
tensorly/decomposition/_cp_power.py	`81.66% <ø> (ø)`
tensorly/decomposition/_symmetric_cp.py	`82.25% <ø> (ø)`
tensorly/tenalg/proximal.py	`100.00% <ø> (ø)`
tensorly/contrib/decomposition/_tt_cross.py	`89.37% <100.00%> (ø)`
...ecomposition/tests/test_mps_decomposition_cross.py	`100.00% <100.00%> (ø)`
tensorly/datasets/synthetic.py	`96.15% <100.00%> (ø)`
tensorly/decomposition/robust_decomposition.py	`100.00% <100.00%> (ø)`
...rly/tenalg/tests/test_generalised_inner_product.py	`100.00% <100.00%> (ø)`
tensorly/tenalg/tests/test_n_mode_product.py	`100.00% <100.00%> (+4.44%)`	⬆️
tensorly/tests/test_cp_tensor.py	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0b701e...36c5710. Read the comment docs.

aarmey · 2021-02-11T04:23:13Z

@JeanKossaifi this should be ready to review when you have a chance.

JeanKossaifi · 2021-02-11T18:22:31Z

Thanks @aarmey, it's great to have this!
Can you merge in the main branch? I fixed an error in the github action script that caused the tests to always be run using TensorLy.

…de-mxnet

aarmey · 2021-02-11T21:02:02Z

Done @JeanKossaifi. MXNet seems to be extra picky about types, so I needed to adjust a few places where ints were being passed in. Everything is working aside from the robust PCA test, which I can't seem to fix. Has something changed about it recently?

JeanKossaifi · 2021-02-11T21:21:19Z

-skip_if_jax = pytest.mark.skipif(tl.get_backend() == "jax",
-                                        reason="Operation not supported in JAX")
+skip_if_backend = pytest.mark.skipif(tl.get_backend() in ("tensorflow", "jax", "mxnet"),
+                                     reason=f"Operation not supported in {tl.get_backend()}")


Why are we skipping for MXNet? I am planning to fix this for JAX and tensorflow too, should be mostly a matter of doing the index assignment with index_update.

Fixed (mostly). See below.

JeanKossaifi · 2021-02-11T21:29:45Z

The issue with the Robust PCA must be due to the changes in the MXNet backend: the test still passes for the other backends and the last changes where made almost a year ago (same for the test, only the random_kruskal was replaced by random_cp). Could it be due to the fact that the dtype is no longer set to float32 by default?

aarmey · 2021-02-11T21:31:05Z

Ok, thanks. I'll try and track down the change...

JeanKossaifi · 2021-02-11T21:42:16Z

If the issue just comes from a dtype issue, then setting the default to None for the dtype as in NumPy might solve the issue. We can then let NumPy/MXNet choose the best dtype based on the input.

aarmey · 2021-02-11T23:34:56Z


        # Delete the selected row
-        mask.pop(max_row_idx)
+        mask.pop(tl.to_numpy(max_row_idx))


MXNet does not automatically make scalars standard Python types, and so this is required to work with pop.

Yes, I was explicitely typecasting in argmax to make up for that strange behaviour.

aarmey · 2021-02-11T23:35:57Z

-@skip_if_tensorflow
+
+@skip_if_backend
+@pytest.mark.skipif(tl.get_backend() in ("mxnet"), reason=f"MXNet bug in advanced indexing (Issue 18919).")


I've gotten the other TT methods to work, but this one has an indexing problem where core is first created. I believe it's the same issue as this Issue on the MXNet repository.

aarmey · 2021-02-11T23:40:02Z

    if mask is None:
        mask = 1
+    else:
+        mask = T.tensor(mask, dtype=float)


MXNet is not properly promoting a boolean/integer-type array when multiplied by a scalar float. The threshold was being converted to 0 below.

That was a tricky one, kuddos for finding/fixing it!
That's a very odd behavior from MXNet -- we could report in the main repo.

Definitely was tricky—I got lucky by seeing rec_X was always 0 with a mask.

apache/mxnet#19891

aarmey · 2021-02-11T23:48:28Z

Everything should be resolved @JeanKossaifi

JeanKossaifi · 2021-02-12T12:11:16Z

-    def argmin(data=None, axis=None):
-        res = nd.argmin(data, axis)
-        if res.shape == (1,):
-            return res.astype('int32').asscalar()


I had added these asscalar and typecasting where appropriate to compensate for the issue with 0-order tensors. Do you think it's better to remove them all?

JeanKossaifi · 2021-02-12T12:15:05Z

Looks good to me overall -- do you think it's better to not do any typecasting or asscalar() in the backend?

aarmey · 2021-02-12T15:34:48Z

Hmm... I'm happy to go with your preference, but think the casting makes more sense this way. I think MXNet is often doing this automatically (e.g., Python's min() works in RPCA) and the only place where an issue arises is with list.pop().

In TT, the mask list could also be implemented as an array, if we made np.delete or its equivalent available as an alternative to list.pop().

It seems more surprising that operations would fundamentally change the type of a variable depending upon the shape you end up with. My mental model of Tensorly is that it provides high-level abstractions of operations that mostly remain within the backend.

JeanKossaifi · 2021-02-12T16:07:47Z

I agree - best to let the backend decide as long as it doesn't break things for us.
Yes, the tt-cross needs some work.

Thanks for the PR, merging!

aarmey added 2 commits February 6, 2021 08:04

Simplify backend checks

796cfde

It works!

f937bbf

aarmey and others added 2 commits February 6, 2021 09:15

Update mxnet_backend.py

c75e011

Merge branch 'master' into upgrade-mxnet

481584e

aarmey added 4 commits February 11, 2021 11:54

Merge branch 'master' into upgrade-mxnet

5df81b7

Merge branch 'upgrade-mxnet' of github.com:aarmey/tensorly into upgra…

a1db10e

…de-mxnet

Fix types

803daf2

Another fix

1448085

JeanKossaifi reviewed Feb 11, 2021

View reviewed changes

Comment thread tensorly/backend/mxnet_backend.py Resolved

Fixes

03ccce8

aarmey commented Feb 11, 2021

View reviewed changes

JeanKossaifi reviewed Feb 12, 2021

View reviewed changes

Comment to reference MXNet issue

36c5710

JeanKossaifi merged commit 4215e3d into tensorly:main Feb 12, 2021

aarmey deleted the upgrade-mxnet branch February 12, 2021 16:17

Conversation

aarmey commented Feb 6, 2021

Uh oh!

codecov Bot commented Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aarmey commented Feb 11, 2021

Uh oh!

JeanKossaifi commented Feb 11, 2021

Uh oh!

aarmey commented Feb 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeanKossaifi commented Feb 11, 2021

Uh oh!

aarmey commented Feb 11, 2021

Uh oh!

Uh oh!

JeanKossaifi commented Feb 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarmey commented Feb 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeanKossaifi commented Feb 12, 2021

Uh oh!

aarmey commented Feb 12, 2021

Uh oh!

JeanKossaifi commented Feb 12, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Feb 6, 2021 •

edited

Loading