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

Permute cp factors #380

Merged
merged 1 commit into from Apr 14, 2022
Merged

Conversation

caglayantuna
Copy link
Member

Permute cp factors

This pull request adds cp_permute_factors function to cp_tensor.py and relevant test to test_cp_tensor.

This function has 2 inputs which are ref_cp_tensor (one cp tensor) and tensors_to_permute (one cp tensor or list of cp tensor). It returns permuted tensor or tensors and permutation list for each permuted tensor.

tl.gather

Since permuting factors require an operator to swap columns, I added tl.gather function to Tensorly to make this function work for tensorflow backend. While using [:, indices] is enough for all the other backends, index operation is not possible with tensorflow unfortunately. I have used take function for numpy, jax, mxnet and indice_select for pytorch backend to have same behaviour for all backends.

Note

I don't see any error with Jax backend in my computer but it fails in github test.

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #380 (fd542aa) into main (a1fa0dc) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #380      +/-   ##
==========================================
- Coverage   88.55%   88.46%   -0.10%     
==========================================
  Files         107      107              
  Lines        6238     6284      +46     
==========================================
+ Hits         5524     5559      +35     
- Misses        714      725      +11     
Impacted Files Coverage Δ
tensorly/cp_tensor.py 85.84% <100.00%> (+1.68%) ⬆️
tensorly/metrics/entropy.py 86.95% <100.00%> (-0.55%) ⬇️
tensorly/tests/test_cp_tensor.py 100.00% <100.00%> (ø)
tensorly/tenalg/proximal.py 67.68% <0.00%> (-3.31%) ⬇️
tensorly/decomposition/_cp.py 89.09% <0.00%> (+1.09%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@caglayantuna
Copy link
Member Author

@cohenjer noticed a function in tensorly which returns permutation with the same scipy function. So, I updated this PR and now using that function but I still need gather to swap the columns for tensorflow.

@JeanKossaifi
Copy link
Member

Thanks, this is great @caglayantuna and @cohenjer.

I am really tempted to either i) skip the tensorflow test for now, like we do e.g. in cross approximation or ii) use the experimental tensorflow numpy API like @aarmey suggested. I don't think it's worth defining gather for all backends just to compensate for tensorflow issue. What do you think?

@aarmey
Copy link
Contributor

aarmey commented Apr 13, 2022

Yes, please, let's skip the tensorflow test for now. The overlap of folks using this with TF will be nil. Given the constant frustration with TF I think it's reasonable to require the experimental interface eventually.

@cyrillustan and @murphymadeleine21, you'll be interested in this.

@caglayantuna caglayantuna reopened this Apr 13, 2022
@caglayantuna
Copy link
Member Author

I updated this PR and skipped tensorflow test. To import congruence_coefficient , I had to use tl.cp_normalize in metrics.entropy instead of importing it to avoid circular import error.

@JeanKossaifi
Copy link
Member

Awesome! This is super useful and we should advertise it to users - @caglayantuna, let's also add it to the API in the doc and perhaps add a few lines in the user-guide? I'll merge this and we can add through another PR?

@JeanKossaifi JeanKossaifi merged commit 3ec46e3 into tensorly:main Apr 14, 2022
@caglayantuna caglayantuna deleted the cp_permute_factors branch April 15, 2022 07:56
@caglayantuna caglayantuna mentioned this pull request May 4, 2022
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