remove type parameter from AbstractTriangular

[simonbyrne]

It isn't used anywhere, and is annoyingly constrained: I came across it while trying to implement a packed triangular, where the backing store is a vector.

[andreasnoack]

Looks like the old version is still used a couple of places

[andreasnoack]

Looks like there are still some there 😄

[andreasnoack]

Bump

[dkarrasch]

I searched the whole Julia project for AbstractTriangular{ with two parameters. Besides those that are fixed in this PR already, there are the following ones:

julia/stdlib/SparseArrays/src/sparseconvert.jl

Line 13 in 1be1deb

const AbstractTriangularSparse{Tv,Ti} = AbstractTriangular{Tv,<:SparseMatrixCSCUnion{Tv,Ti}}

If the type gets removed, this would need to be replaced by a Union over all subtypes of AbstractTriangular, whose second parameter is a subtype of SparseMatrixCSC. Unfortunately, that may lead to method ambiguities, IIUC.

Then the first and the last lines here:

julia/stdlib/SparseArrays/src/sparsevector.jl

Lines 1026 to 1031 in 1be1deb

	const _Triangular_SparseConcatArrays{T,A<:_SparseConcatArrays} = LinearAlgebra.AbstractTriangular{T,A}
	const _Annotated_SparseConcatArrays = Union{_Triangular_SparseConcatArrays, _Symmetric_SparseConcatArrays, _Hermitian_SparseConcatArrays}
	const _Symmetric_DenseArrays{T,A<:Matrix} = Symmetric{T,A}
	const _Hermitian_DenseArrays{T,A<:Matrix} = Hermitian{T,A}
	const _Triangular_DenseArrays{T,A<:Matrix} = LinearAlgebra.AbstractTriangular{T,A}

There are some tests in subtype.jl, but they seem to be independent from the LinearAlgebra structs.

[dkarrasch]

Since the second parameter does get used in other place, maybe it would be a better option to keep it but relax or remove the restriction?

[dkarrasch]

Once JuliaSparse/SparseArrays.jl#329 is merged and the stdlib is bumped, this PR should pass tests and we can run a pkgeval test.

[dkarrasch]

The failing tests are unrelated. Let's run a pkgevaluation.

@nanosoldier runtests()

[nanosoldier]

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

[dkarrasch]

This needs JuliaArrays/StaticArrays.jl#1136 to be merged and released, and then another pkgeval run.

[dkarrasch]

@nanosoldier runtests()

[nanosoldier]

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

[dlfivefifty]

I think you should make Bidiagonal <: AbstractTriangular after this change.

[dkarrasch]

Interesting idea. And Diagonal as well? After my recent work on AbstractQ and Factorizations, I keep considering abstract types in LinearAlgebra w.r.t. an API. What is needed for any subtype? Or conversely, why should people make their types subtype something? If they don't use any fallbacks (or if such don't even exist) and write out all methods that they consider necessary, why would they subtype Factorization or AbstractTriangular? In ArrayLayouts.jl, are Bidiagonals treated as triangular in any (trait) way?

EDIT: BTW, I'm not argueing against subtyping, I'm argueing in favor of developing nice APIs for these abstract types and reduce the code in packages.

[dlfivefifty]

Ha good question. In ArrayLayouts.jl both DiagonalLayout and BidiagonalLayout are subtypes of AbstractBandedLayout, that is, their sparsity trumps their triangularness. Even with the trait-based approach its not clear how to handle "multiple inheritance"...

So perhaps ignore my suggestion (for now).

[dkarrasch]

@nanosoldier runtests()

[nanosoldier]

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

[dkarrasch]

@nanosoldier runtests(["InterpolatedRejectionSampling", "GroundMotion", "Walker2020Sampling", "DataFitting", "AES", "CorrectMatch", "ActuarialScience", "IntervalConstraintProgramming", "Firefly", "SeisProcessing", "GARCH", "LiteQTL", "ShellModel", "Flux3D", "SimpleCarModels", "JudiLing", "MolecularGraph", "ImageComponentAnalysis", "Jaynes", "AgnosticBayesEnsemble", "OceanColorData", "EchogramImages", "BATBase", "EvolutionaryModelingTools", "EFTfitter", "ZeroInflatedLikelihoods", "SparseTransforms", "GlobalSearchRegression", "IncrementalPruning", "DistributedFlux", "HawkesProcesses", "Changepoints", "QSimulator", "Harlequin", "HDF5Utils", "GLFixedEffectModels", "JOLI", "StochasticProcesses", "EBayes", "SpatialGraphs", "PositionVelocityTime", "MeshPorter", "OptimalTransmissionRouting", "ThreadedSparseCSR", "Judyp", "MotifRecognition", "LITS", "Biomodelling", "Crystallography", "BayesianIntegral", "MLJJLBoost", "WorldOceanAtlasTools", "ConScape", "TopOptMakie", "PartiallySeparableSolvers", "LatticeSites", "JLBoostMLJ", "AsyPlots", "CharacteristicInvFourier", "CompEcon", "DynamicLinearModels", "PulsarSearch", "StanMamba", "VectorizedRoutines", "SubSIt", "SQLStore", "Metadata", "QXTns", "SeaPearlZoo", "Phylo", "RandomQuantum", "MultiUAVDelivery", "FractionalGaussianFields", "SumProductNetworks", "MPIReco", "RandomCorrelationMatrices", "Cliffords", "InvariantCausal", "StructuralDynamicsODESolvers", "ProgenyTestingTools", "PixelArt", "SeparatingAxisTheorem2D", "MoistAir", "MetapopulationDynamics", "MeshKeeper", "Bridge", "SideKicks", "GroupedErrors", "JET", "ExtensibleEffects", "BAT", "Multitaper", "ZonalFlow", "NumericalMethodsforEngineers", "NetworkInference", "PlotShapefiles", "DickeModel", "JCheck", "MDBM", "CollectiveSpins", "PartialSvdStoch", "QuaternionIntegrator", "MzPlots", "QuantumInfo", "YAXArrayBase", "LASindex", "RatingCurves", "Spectra", "FractionalDelayFilter", "RandomFourierFeatures", "ConvolutionalOperatorLearning", "ASE", "FluxExtra", "DetectionTheory", "DeformableBodies", "InterpolatedPDFs", "SemiDiscretizationMethod", "AeroAcoustics", "Hwloc", "Gloria", "GCMAES", "BenchmarkEnvironments", "PolyesterForwardDiff", "Immerse", "ChainPlots", "Recommenders", "ComplexRegions", "Mamba", "LinearCovarianceModels", "MultiStateSystems", "GrowthMaps", "PointerStructs", "MultipathChannel", "DeconvOptim", "MultiStochGrad", "RainFARM", "DifferentialDynamicsModels", "Mitosis", "MakieLayout", "NeuralQuantumState", "Layered", "LogisticOptTools", "BlobTracking", "MolecularBoxes", "ANOVA", "ArnoldiMethodTransformations", "MarriageMarkets", "MayOptimize", "TropicalYao", "DPClustering", "SpikeSorting", "DateShifting", "DungBase", "PosDefManifoldML", "KVectors", "RvSpectML", "ChemometricsTools", "KiteModels", "TheCannon", "MeshFinder", "LinearInterpolators", "ContinuousTransformations", "PPLM", "SDFResults", "StochasticSemiDiscretizationMethod", "FinancialDerivatives", "ValidatedNumerics", "ReverseMcCormick", "ModelConstructors", "UNet", "StateSpaceReconstruction", "GLTF", "TensorPolynomialBases", "Reinforce", "PyBraket", "LoggingFacilities", "BridgeDiffEq", "StaticArrays", "ZeroInflatedDistributions", "ParticleMDI", "NablaNet", "EmpiricalModeDecomposition", "Dyn3d", "LazyAlgebra", "DecisionMakingEnvironments", "CounterfactualExplanations", "MoziFESection", "ClimaComms", "PProf", "FactoredValueMCTS", "ObservationSchemes", "BangBang", "MeshMaker", "SpinMonteCarlo", "GridDensities", "RecordedArrays", "ComplexPhasePortrait", "RvLineList", "LinearMixingModels", "LighthouseFlux", "MCMCDebugging", "Clapeyron"])

[nanosoldier]

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

[dkarrasch]

The remaining failures are either due to compat restrictions on FillArrays.jl or on ArrayInterface.jl, and (seemingly) mostly indirectly, so for different reasons. They are also stuck at different minor version in the v0.x series, so it's a nightmare to track all that down. Some maintainers accept the changes but then don't release the update... So, what's the policy for situations like this? A bit of a relief could be to have a FillArrays@v0.12.x backport, because some have already missed the v0.13 bump.

[dkarrasch]

Sorry for the "review spam", but I'm trying to push this towards a decision. The current state is that this PR breaks packages that have very old compat restrictions, mostly, if not all, related to StaticArrays.jl (v0.12, so they missed v0.13, not to mention the upgrade to v1.x). Would it be acceptable to still merge this? If not, we should perhaps close this, or do we have some infrastructure to nudge package maintainers to upgrade their compat entries?

[vtjnash]

We do have some infrastructure for that: you can make a PR to the General repo that adds a compat bound to StaticArrays <v0.12 that restricts it to julia <v1.10

[dkarrasch]

That means we can safely merge this and then look for somebody who knows how to do registry manipulations? 😉

[dkarrasch]

@KristofferC I'm merging this, and suggest we backport this to v1.10. In the backport (if we do it), we need to add a note the NEWS.md:

* The abstract type `AbstractTriangular` now only has one type parameter `T`, indicating its
  `eltype`. The second parameter `S<:AbstractMatrix` used to encode the type information on
  the data container. To refer to the union of `[Unit][Upper/Lower]Triangular` types,
  use `LinearAlgebra.UpperOrLowerTriangular` ([#26307]).

How do we do that? Open a PR against the backport branch that adds only the NEWS entry?

[maleadt]

This also seems to have broken ArrayLayouts, and thus ArrayInterface, which a significant number (250+) of packages relies on.

[dlfivefifty]

Probably just needs this line to 4 different overloads (Unit/UpperTriangular, Unit/LowerTriangular):

https://github.com/JuliaLinearAlgebra/ArrayLayouts.jl/blob/a7786a0e740bac5e5befbcd342cdd015b501ab04/src/ArrayLayouts.jl#L151

[dkarrasch]

I'm very sorry, I must have overlooked that in the pkgeval runs. As indicated in the news entry, we need to replace AbstractTriangular by UpperOrLowerTriangular. Then we can use two type parameters.

Also, I realized that I might have merged this too early. Should we revert this, reland it and run pkgeval after the registry PRs are merged? Then the pkgeval picture should be much clearer (many packages with very old compat bounds will simply stop to work on v1.10/11). Sorry again!!!

[oscardssmith]

This also breaks https://github.com/TuringLang/DistributionsAD.jl/blob/master/ext/DistributionsADTrackerExt.jl