Skip to content

RFC: Introduce AdjointRotation to avoid subtyping AbsMat#46233

Merged
dkarrasch merged 6 commits intomasterfrom
dk/rotation
Sep 6, 2022
Merged

RFC: Introduce AdjointRotation to avoid subtyping AbsMat#46233
dkarrasch merged 6 commits intomasterfrom
dk/rotation

Conversation

@dkarrasch
Copy link
Copy Markdown
Member

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 dkarrasch added breaking This change will break code linear algebra Linear algebra linalg triage labels Jul 31, 2022
@dkarrasch
Copy link
Copy Markdown
Member Author

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

@nanosoldier
Copy link
Copy Markdown
Collaborator

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

@dkarrasch
Copy link
Copy Markdown
Member Author

@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 dkarrasch marked this pull request as ready for review August 22, 2022 16:25
@dkarrasch
Copy link
Copy Markdown
Member Author

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")

@nanosoldier
Copy link
Copy Markdown
Collaborator

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

@dkarrasch
Copy link
Copy Markdown
Member Author

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")

@nanosoldier
Copy link
Copy Markdown
Collaborator

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

@dkarrasch
Copy link
Copy Markdown
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking This change will break code linear algebra Linear algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants