-
Notifications
You must be signed in to change notification settings - Fork 281
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
Feature/mda #206
base: main
Are you sure you want to change the base?
Feature/mda #206
Conversation
Thanks @james-oldfield! On a high-level, would it make sense to also have an additional solver based on SVD (e.g. like for the linear case in scikit-learn)? This would avoid building the covariance matrix (it seems we make the assumption that all classes have the same covariance anyway here?). However, it's not clear whether it would be computationally advantageous in the end. Looking at the update rule, an online version might be nice to add down the line. As a side note, it's good to make the docstring as easily readible as possible (e.g. not too much latex) -- we can have a short explanation in the docstring itself and have a longer, more math heavy version in the user guide. |
Definitely! Although off the top of my head I'm not quite sure how one would extend the SVD solver for the multilinear case (i.e. how we'd solve for the factor matrices with the SVD directly on the batch of mode-n unfolding (a 3rd-order tensor), without computing the scatter matrices explicitly?) It certainly would make a nice optional feature though if it were possible. One thing that would be nice in the future could be to provide a flag to instead take the solution via the eigendecomposition of Good idea with the docstring, I've removed a lot of the bulk :) |
############### | ||
# check correct # of ranks have been supplied | ||
############### | ||
assert len(ranks) == len(T.shape(X)[1:]), 'Expected number of ranks: {}. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good for end users to make the error messages as informative as possible. Something like:
assert len(ranks) == len(T.shape(X)[1:]), 'Expected number of ranks: {}. \ | |
if len(ranks) != (T.ndim(X) - 1): | |
msg = f'Got as input a tensor of shape {T.shape(X)} corresponding to {T.shape(X)[0]} samples of shape {T.shape(X)[1:]} of order {T.ndim(X) -1}.' | |
msg += f'But got {len(ranks)} ranks != {T.ndim(X) -1}.' | |
raise ValueError(msg) |
# store the mean of all tensor samples with label i | ||
for i in range(len(set(y))): | ||
# tensorflow is only backend to not support indexing into tensor with a list | ||
if backend == 'tensorflow': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like/want to have these in the code (it should be backend agnostic). However tensorflow really does not make it easy.. Would be ideal to find another way if it doesn't slow down the other backends.
if backend == 'tensorflow': | ||
class_means += [T.mean(T.tensor([X[j] for j in class_idx[i]]), axis=0)] | ||
else: | ||
class_means += [T.mean(X[class_idx[i], ...], axis=0)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can directly get the means without creating class_idx
:
class_means += [T.mean(X[class_idx[i], ...], axis=0)] | |
class_means += [T.mean(X[y == i, ...], axis=0)] |
Of course tensorflow doesn't support this directly but through a function:
tf.boolean_mask(X, y==i)
equivalently, for TensorFlow, we can do:
tl.stack([X[j, ...] for j, e in enumerate(tl.to_numpy(y)) if e == i], 0)
# -- | ||
# [1] Q. Li et al. "Multilinear Discriminant Analysis for Higher-Order Tensor Data Classification" | ||
################################################### | ||
U, _, _ = T.partial_svd(T.dot(W_scat_inv, B_scat)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably be using SVD_FUNS but this is being refactored in #217 and should be easier to use when that gets merged.
return factors | ||
|
||
|
||
def compute_modek_wb_scatters(X, mode, factors, global_mean, class_means, class_idx): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about naming it mode_scatter_matrices
? That would be consistent with mode_dot
.
Or even just scatter_matrices
and we document well that this corresponds to mode-n between and within class scatter matrices?
# recover X, using the inverse projection matrices | ||
X_hat = multi_mode_dot(Z, [tl.tensor(inv(tl.to_numpy(f))) for f in factors], modes=[1, 2, 3], transpose=True) | ||
|
||
assert_array_almost_equal(X, X_hat, decimal=tol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this always true as long as the factors are not singular? Also the factors are obtain through SVD so orthogonal, we can just take their transpose instead of converting to NumPy and using inv
. It would be good to have some specific tests.
d45beab
to
2aa9834
Compare
Also opening a PR for Li's constrained multilinear discriminant analysis [1]. This could also perhaps suit
tensorly/decomposition
best, given the current structure? Along with MPCA, there's also a notebook walkthrough that could potentially fit into the examples!