Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Introduce AdjointRotation to avoid subtyping AbsMat #46233

Merged
merged 6 commits into from
Sep 6, 2022
Merged

Conversation

dkarrasch
Copy link
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
Member Author

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

@nanosoldier
Copy link
Collaborator

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

@dkarrasch
Copy link
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
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
Collaborator

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

@dkarrasch
Copy link
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
Collaborator

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

@dkarrasch
Copy link
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 linalg triage linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants