julia icon indicating copy to clipboard operation
julia copied to clipboard

RFC: Introduce `AdjointRotation` to avoid subtyping `AbsMat`

Open dkarrasch opened this issue 3 years ago • 3 comments

This is the analogue to #46196 for AbstractRotation. It is somewhat weird that Adjoint{<:Any,<:NOTAbstractMatrix} still subtypes to AbstractMatrix, which introduces many ambiguities. Typically, non-matrices like Q's and rotations should have preference over any AbstractMatrix in multiplication. That is, no matter how structured/sparse the other factor is, the q-multiplication code or the rotation-code should apply, not the other way around. Technically, this is again breaking, but I expect even less friction than in #46196.

dkarrasch avatar Jul 31 '22 13:07 dkarrasch

@nanosoldier runtests(ALL, vs = ":master")

dkarrasch avatar Jul 31 '22 14:07 dkarrasch

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

nanosoldier avatar Aug 02 '22 05:08 nanosoldier

@nanosoldier runtests(["ASE", "ArgoData", "Bagyo", "BugReporting", "COPT", "CVRPLIB", "CairoMakie", "Cambrian", "CellListMap", "Clang", "ClimaCorePlots", "CommunityDetection", "CompactBasisFunctions", "ControlSystems", "CountdownNumbers", "CrystalInfoFramework", "DataDeps", "DrelTools", "FameSVD", "FlameGraphs", "Folds", "Gen", "GenericLinearAlgebra", "GenericSchur", "GeometricIntegrators", "GeometricIntegratorsDiffEq", "GraphMLDatasets", "GraphNeuralNetworks", "Hashpipe", "HiddenMarkovModelReaders", "ITensorGaussianMPS", "IncrementalPruning", "InformationGeometry", "KernelEstimator", "LatticeDiracOperators", "LogicToolkit", "LoopThrottle", "LowRankIntegrators", "Lux", "MatrixMarket", "McCormick", "MultivariatePolynomials", "NEOSServer", "NNlib", "NeighbourLists", "Nonconvex", "ParameterSpacePartitions", "PerronFrobenius", "Pfam", "Pidfile", "PolaronPathIntegrals", "PoreMatMod", "ProgressiveHedging", "ProxSDP", "QuadraticToBinary", "QuadratureRules", "Quiqbox", "ReinforcementLearningExperiments", "RetroCap", "RungeKutta", "StableTrees", "StochasticDelayDiffEq", "StochasticIntegrators", "StochasticPrograms", "StrBase", "StressTest", "SymbolicRegression", "TopoPlots", "TuringGLM", "Variography", "VideoIO"], vs = ":master")

dkarrasch avatar Aug 02 '22 18:08 dkarrasch

Shall we consider this? I'll try to run the tests once again, since earlier issues have been resolved meanwhile, I believe.

@nanosoldier runtests(["ASE", "ArgoData", "Bagyo", "BugReporting", "COPT", "CVRPLIB", "CairoMakie", "Cambrian", "CellListMap", "Clang", "ClimaCorePlots", "CommunityDetection", "CompactBasisFunctions", "ControlSystems", "CountdownNumbers", "CrystalInfoFramework", "DataDeps", "DrelTools", "FameSVD", "FlameGraphs", "Folds", "Gen", "GenericLinearAlgebra", "GenericSchur", "GeometricIntegrators", "GeometricIntegratorsDiffEq", "GraphMLDatasets", "GraphNeuralNetworks", "Hashpipe", "HiddenMarkovModelReaders", "ITensorGaussianMPS", "IncrementalPruning", "InformationGeometry", "KernelEstimator", "LatticeDiracOperators", "LogicToolkit", "LoopThrottle", "LowRankIntegrators", "Lux", "MatrixMarket", "McCormick", "MultivariatePolynomials", "NEOSServer", "NNlib", "NeighbourLists", "Nonconvex", "ParameterSpacePartitions", "PerronFrobenius", "Pfam", "Pidfile", "PolaronPathIntegrals", "PoreMatMod", "ProgressiveHedging", "ProxSDP", "QuadraticToBinary", "QuadratureRules", "Quiqbox", "ReinforcementLearningExperiments", "RetroCap", "RungeKutta", "StableTrees", "StochasticDelayDiffEq", "StochasticIntegrators", "StochasticPrograms", "StrBase", "StressTest", "SymbolicRegression", "TopoPlots", "TuringGLM", "Variography", "VideoIO"], vs = ":master")

dkarrasch avatar Aug 22 '22 16:08 dkarrasch

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

nanosoldier avatar Aug 23 '22 08:08 nanosoldier

The deprecated ITensorsGaussianMP.jl fails, but its up-to-date version in ITensors.jl seems to pass. Let's look at that specifically. I wonder why ITensors.Circuit actually subtypes AbstractRotation, if all methods are provided explicitly anyway, i.e., no fallbacks are used.

@nanosoldier runtests(["ITensors"], vs = ":master")

dkarrasch avatar Aug 23 '22 12:08 dkarrasch

Your package evaluation job has completed - no new issues were detected. A full report can be found here.

nanosoldier avatar Aug 23 '22 21:08 nanosoldier

Since, after all, this doesn't seem to break any code, I'll merge in a few days if no objections arise. This part of the code seems to be mainly used as a blackbox, without any extensions/overloads/etc. (with ITensors.jl the only exception, which in turn doesn't use any fallback definitions but instead provides all required functions itself). @emstoudenmire @mtfishman @kshyatt

dkarrasch avatar Aug 29 '22 09:08 dkarrasch