Repository navigation
Add testing for complex values in CP - #423
Conversation
yngvem
left a comment
yngvem
left a comment
There was a problem hiding this comment.
It seems like I forgot to submit my review last week, so some of my comments may be outdated... I cannot check if that's the case now, as I'm out travelling, but hopefully they should be helpful :)
| @pytest.mark.parametrize("init", ["svd", "random"]) | ||
| @pytest.mark.parametrize("normalize_factors", [False, True]) | ||
| @pytest.mark.parametrize("random_state", [1, 1234]) | ||
| @pytest.mark.parametrize("random_state", [1, 1236]) |
There was a problem hiding this comment.
I changed the random seed here because the random initialization is pathologically bad. It works fine (and I directly checked the MTTKRP error calculation) but ALS gets trapped without line search or orthogonalization.
There was a problem hiding this comment.
That's awesome, thanks @aarmey!
I wonder if we could write a small tool to automatically check for such pathological cases?
There was a problem hiding this comment.
Interesting idea. It should be clear by looking at cp_lstsq_grad(). It'd be too expensive to run every iteration, but is certainly something you could run at the end to look for issues.
Codecov Report
@@ Coverage Diff @@
## main #423 +/- ##
==========================================
+ Coverage 88.55% 88.58% +0.02%
==========================================
Files 108 108
Lines 6423 6428 +5
==========================================
+ Hits 5688 5694 +6
+ Misses 735 734 -1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
Thanks @aarmey! I love how you also casually improved the quality of life for the whole library in the process by fixing the test and speeding up the tests for JAX and TF! |
|
If someone can give this a last look, it should be ready and all the tests are passing now. |
JeanKossaifi
left a comment
JeanKossaifi
left a comment
There was a problem hiding this comment.
Awesome, looks good to me!
| # mttkrp and factor for the last mode. This is equivalent to the | ||
| # inner product <tensor, factorization> | ||
| iprod = tl.sum(tl.sum(mttkrp * factors[-1], axis=0)) | ||
| iprod = tl.sum(tl.sum(mttkrp * tl.conj(factors[-1]), axis=0)) |
There was a problem hiding this comment.
Why does it have to be the conjugate here?
There was a problem hiding this comment.
@yngvem had this insight, so he might be able to explain. I confirmed that the results are indeed wrong without this.
|
@aarmey @JeanKossaifi I wasn't able to find the button for submitting my comments after it was merged, so I'm answering the mttkrp-question here :) Since I think we could conjugate the mttkrp instead of the factor matrix, but I would have to check. Anyways, the point is that we need at least one conjugate on that line for there to be a complex inner product |
|
Thanks for the clear answer @yngvem! Given that you went through the effort of writing it so nicely, I'd say let's add it to the docstring (in notes) or user guide, what do you think? |
Here is a basic test setup for complex valued tensors. I will add comments documenting the failure cases.
Addresses #422