Skip to content

implement test_approx for Eigen#163

Closed
simeonschaub wants to merge 1 commit intomasterfrom
sds/test_approx_eigen
Closed

implement test_approx for Eigen#163
simeonschaub wants to merge 1 commit intomasterfrom
sds/test_approx_eigen

Conversation

@simeonschaub
Copy link
Member

This is needed for JuliaDiff/ChainRules.jl#431.

@simeonschaub
Copy link
Member Author

@sethaxen Is this strong enough for the eigen tests in ChainRules? Or do we actually want to require the eigenvectors to be approximately equal as well?

# Comparing eigen factoriztions is tricky, because the order and normalization of eigen
# vectors is not unique. Therefore we just calculate the original matrix and compare that
# instead.
function test_approx(actual::Eigen, expected::Eigen, msg=""; kwargs...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should do this for all subtypes of Factorization ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yes, maybe? Although I am not really familiar with those, so perhaps we should wait until an actual use case comes up?

@oxinabox oxinabox requested a review from sethaxen June 2, 2021 16:21
@oxinabox
Copy link
Member

oxinabox commented Jun 2, 2021

I will let @sethaxen give final approval on this, as I don;t know how things work with Eigen

@sethaxen
Copy link
Member

sethaxen commented Jun 8, 2021

This is to be used to test that the primal part of frule and rrule returns the same decomposition as the primal function, right? In that case, I think we do care whether the eigenvalues and eigenvectors match. But also, it should not be hard to make them match, right, since the eigen rules call eigen internally? Perhaps I'm missing something.

@simeonschaub
Copy link
Member Author

Hmm, yes you might be right. Looking at the failures in https://github.com/JuliaDiff/ChainRules.jl/runs/2769420821?check_suite_focus=true#step:6:282 for example again, it looks like the values shown do actually match precisely, any ideas what could cause this? There shouldn't be any NaN's occuring in those tests, right? Even if there would be degeneracy, that shouldn't cause the results to be non-deterministic no? I can look into this some more if you don't have any ideas either.

@sethaxen
Copy link
Member

sethaxen commented Jun 8, 2021

it looks like the values shown do actually match precisely, any ideas what could cause this? There shouldn't be any NaN's occuring in those tests, right?

I'm not certain. But I do see some undef's in that failed test. Is it possible a matrix has undefined values?

@simeonschaub
Copy link
Member Author

I only see that for the arrays with 0 elements, but I could be wrong.

@simeonschaub
Copy link
Member Author

simeonschaub commented Jun 8, 2021

Ah, wait! It seems like Eigen now also contains condition numbers which are just random noise in this instance. That's probably why tests fail.

@simeonschaub simeonschaub deleted the sds/test_approx_eigen branch June 8, 2021 16:06
oxinabox pushed a commit that referenced this pull request Jul 21, 2021
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.

3 participants