Miscellaneous housekeeping improvements #513
Conversation
| ones, | ||
| zeros, | ||
| zeros_like, | ||
| eye, | ||
| where, | ||
| conj, |
There was a problem hiding this comment.
These methods were not being exported, and I didn't see a reason for being left out.
Codecov Report
@@ Coverage Diff @@
## main #513 +/- ##
==========================================
+ Coverage 86.71% 86.78% +0.07%
==========================================
Files 120 118 -2
Lines 7535 7515 -20
==========================================
- Hits 6534 6522 -12
+ Misses 1001 993 -8
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| """ | ||
| raise NotImplementedError | ||
|
|
||
| @staticmethod | ||
| def min(tensor): | ||
| def min(tensor, axis=None): |
There was a problem hiding this comment.
The axis argument is actively used, so it should be represented here.
| from . import backend as tl | ||
| from .utils import prod |
There was a problem hiding this comment.
prod was only here for Python v3.8 support.
| @@ -426,8 +426,6 @@ def parafac( | |||
| tl.solve(tl.conj(tl.transpose(pseudo_inverse)), tl.transpose(mttkrp)) | |||
| ) | |||
| factors[mode] = factor | |||
| if normalize_factors and mode != modes_list[-1]: | |||
There was a problem hiding this comment.
I think the decision to do this only at the end came out of #264, but was in a closed PR. I need to dig it up.
| K = 8 | ||
| M = 7 | ||
| K = 16 | ||
| M = 14 |
There was a problem hiding this comment.
To help with occasional testing failures.
| # Normalize separately—this was profiling very slow in parafac | ||
| Z_comp = cp_normalize(Z_comp_CP)[1] | ||
| for mode in range(len(Z_comp)): | ||
| factor = multi_mode_dot(Z, Z_comp, skip=mode) |
There was a problem hiding this comment.
Tucker is equivalent to CP when rank 1. This is more performant, and this spot was consistently one of the slowest parts of testing, particularly for indexing-slow backends.
There was a problem hiding this comment.
What was the main bottleneck in cp_normalize? Was it just the part making sure the scales are non-zero?
Or was the slow part just calling the CP where it's not needed?
There was a problem hiding this comment.
Here, I think this mostly just came down to calling CP, with a lot of extra code contained within, when it is not strictly needed. This call is inside two loops, and so it can be called thousands of times. Within the profiling, I believe most of the time was spent on the MTTKRP, but other parts of the CP call weren't negligible.
| ) | ||
| V = tl.index_update(V, tl.index[k, :], V[k, :] + deltaV) | ||
| # Modifying the function for sparsification | ||
| if sparsity_coefficient is not None: |
There was a problem hiding this comment.
This code should be unchanged, but (1) reuses some indexing results for indexing-slow backend performance, and (2) I find it a bit more readable by removing repetitive segments and breaking the expressions up into logical units.
There was a problem hiding this comment.
Agreed, this is much more readable and concise
| tl.index[i, j, k], | ||
| tl.sum(product * tl.eye(product.shape[0])), | ||
| ) | ||
| tensor = tl.einsum("iaj,jbk,kci->abc", *factors) |
There was a problem hiding this comment.
We have trace in the backend now (see TODO comment), but I thought this was cleaner.
There was a problem hiding this comment.
Thanks @aarmey! Always great to have improvements and more readable code!
| # Normalize separately—this was profiling very slow in parafac | ||
| Z_comp = cp_normalize(Z_comp_CP)[1] | ||
| for mode in range(len(Z_comp)): | ||
| factor = multi_mode_dot(Z, Z_comp, skip=mode) |
There was a problem hiding this comment.
What was the main bottleneck in cp_normalize? Was it just the part making sure the scales are non-zero?
Or was the slow part just calling the CP where it's not needed?
tensorly/decomposition/_tr.py
Outdated
| @@ -216,7 +216,7 @@ def tensor_ring_als( | |||
|
|
|||
| if ls_solve == "lstsq": | |||
| # Solve least squares problem directly | |||
| sol, _ = tl.lstsq(design_mat, tensor_unf) | |||
| sol = tl.lstsq(design_mat, tensor_unf)[0] | |||
There was a problem hiding this comment.
Is it better to do this? If so why not *_?
There was a problem hiding this comment.
I didn't know *_ was possible! Agreed this will be more readable.
| ) | ||
| V = tl.index_update(V, tl.index[k, :], V[k, :] + deltaV) | ||
| # Modifying the function for sparsification | ||
| if sparsity_coefficient is not None: |
There was a problem hiding this comment.
Agreed, this is much more readable and concise
tensorly/tests/test_backend.py
Outdated
| @@ -407,21 +407,21 @@ def test_lstsq(): | |||
| # test dimensions | |||
| a = T.randn((m, n)) | |||
| b = T.randn((m, k)) | |||
| x, res = T.lstsq(a, b) | |||
| assert_equal(x.shape, (n, k)) | |||
| ret = T.lstsq(a, b) | |||
There was a problem hiding this comment.
Same question as above for those: I find it typically slightly more readible to explicitly catch return values or use _ rather than index tuples
Apologies that this does not involve just one change. This resolves some various minor inconsistencies, deprecations, and performance issues:
np.lstsqinterface, so all four return values are passed on nowpytest-randomly. This sets the random seeds of the default random generators and reports it in output. I've found this is very helpful when dealing with inconsistent errors.I will further annotate each of the changes so that this is simpler to review.