fix some issues with equality of factorizations

[simeonschaub]

• hash did not respect the type of a factorization, so completely
  different factorizations with the same underlying data would result in
  same hash leading to inconsistencies with isequal. This likely
  doesn't occur very often in practice, but definitely seems worth
  fixing.
• == and isequal only returned true if two factorizations are of
  exactly the same type, which is inconsistent with their implementation
  for other objects and with the definition of hash for factorizations.
• Equality for QRCompactWY did not ignore the subdiagonal entries of
  T leading to nondeterministic behavior. Perhaps T should be
  directly stored as UpperTriangular in QRCompactWY, but that seems
  potentially breaking.

Relying on implementation details of DataType here is certainly less
than ideal, but I could not come up with a nicer solution.

[simeonschaub]

This needs to be manually changed to qr(x, Val(true)) when backporting this to 1.6. @KristofferC what is the best way to ensure this? Should I maybe remove the backport-1.6 label here and open a new PR with that change against the release-1.6 branch?

[andreasnoack]

I've been told that you should never fetch the internal fields like this. Maybe you can hash propertynames instead?

More generally, should the hash traverse propertynames instead of the fields? That should avoid the issue with non-active memory affecting the hash and maybe make the specialized methods redundant.

[simeonschaub]

I've been told that you should never fetch the internal fields like this. Maybe you can hash propertynames instead?

I agree it's a bit ugly, but I am unsure just using propertynames for the hash is quite enough, since a lot of them like vectors and values for Eigen are quite generic and not necessarily unique. Perhaps that is being a bit too paranoid, but hashing by type identity seems more formally correct, at least to me. For a stdlib, relying on internals is probably also not quite as bad, since it will always get tested and updated alongside other Base changes.

More generally, should the hash traverse propertynames instead of the fields?

Yeah, maybe? I think that would mean hashing the same data twice for some factorizations though, right?

[andreasnoack]

I agree it's a bit ugly

I don't think the reason to avoid it was aesthetics but it's really not my department. Others should comment that if it should be avoided. Maybe it used to be a problem but no longer is. Otherwise, let's just keep it.

I think that would mean hashing the same data twice for some factorizations though, right?

Yes. For e.g. Cholesky it would probably end up hashing the same data three times so we should probably have specialized versions. However, hashing the fields seems wrong since the inactive memory will affect the hash so I think using the properties will be the correct, although slow, fallback. E.g.

julia> A = Symmetric(randn(3,3) + 10I, :U)
3×3 Symmetric{Float64, Matrix{Float64}}:
 8.52303    0.491492   0.311278
 0.491492   9.43415   -1.63795
 0.311278  -1.63795    9.80197

julia> Ac = copy(A);

julia> Ac.data[3,1] += 1
1.9147549806916584

julia> hash(cholesky(A)) == hash(cholesky(Ac))
false

julia> cholesky(A).U == cholesky(Ac).U
true

[simeonschaub]

I think the main reason we discourage use of internal APIs like this is because they might change at any time, which will break such code. I don't think there are any other glaring problems with this approach.

Using propertynames for the fallback seems reasonable, I will change that.

[simeonschaub]

Hmm, trying this locally, using propertynames fails for bunchkaufman.

For e.g. Cholesky it would probably end up hashing the same data three times so we should probably have specialized versions.

It seems like this is an issue for quite a lot of factorizations, so if we want this to be efficient, we would end up having to define this manually for a lot of cases, which kind of defeats the purpose of having this definition in the first place.

The more I think about it, the more I am convinced that we should just remove these definitions for the abstract type Factorization, since they really don't make much sense in terms of the abstract Factorization interface, but instead have a macro similar to https://github.com/andrewcooke/AutoHashEquals.jl, which defines these for each factorization separately. That would also avoid having to rely on F.name.wrapper. What do you think? It might be somewhat breaking if users define their own factorization types though.

[simeonschaub]

Bump. How do you think we should proceed here?

[andreasnoack]

Wouldn't the macro based version end up inspecting the fields which was what I argued against in #41228 (comment)