SciML / LinearSolve.jl

LinearSolve.jl: High-Performance Unified Interface for Linear Solvers in Julia. Easily switch between factorization and Krylov methods, add preconditioners, and all in one interface.
https://docs.sciml.ai/LinearSolve/stable/
Other
248 stars 53 forks source link

make issquare extend SciMLOperators.issquare #269

Closed vpuri3 closed 1 year ago

vpuri3 commented 1 year ago

close https://github.com/SciML/LinearSolve.jl/issues/268

codecov[bot] commented 1 year ago

Codecov Report

Merging #269 (2190beb) into main (af09c4e) will increase coverage by 32.23%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main     #269       +/-   ##
===========================================
+ Coverage   38.51%   70.74%   +32.23%     
===========================================
  Files          13       14        +1     
  Lines         810      841       +31     
===========================================
+ Hits          312      595      +283     
+ Misses        498      246      -252     
Impacted Files Coverage Δ
src/LinearSolve.jl 100.00% <ø> (+25.00%) :arrow_up:
src/common.jl 89.83% <100.00%> (+5.34%) :arrow_up:
src/default.jl 50.64% <100.00%> (+14.28%) :arrow_up:
src/HYPRE.jl 100.00% <0.00%> (ø)
lib/LinearSolvePardiso/src/LinearSolvePardiso.jl 75.00% <0.00%> (+2.08%) :arrow_up:
src/iterative_wrappers.jl 77.31% <0.00%> (+23.43%) :arrow_up:
src/preconditioners.jl 86.66% <0.00%> (+26.66%) :arrow_up:
... and 3 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

vpuri3 commented 1 year ago

@ChrisRackauckas this is done

vpuri3 commented 1 year ago

Why can't/isn't this just an internal method?

We're integrating SciMLOperators, which will be replace DiffEqOperators, DiffEqArrayOperators, etc. As it will be used to represent linear operators, and LinearSolve solves linear systems, the two packages need to support each other. SciMLOperators will be upstream of SciMLBase, and will be reexported by it. We already have SciMLOperators.issquare as a utility. Check: https://github.com/SciML/SciMLOperators.jl/blob/master/src/interface.jl

help?> SciMLOperators.issquare                                                         
  No documentation found.                                    
  SciMLOperators.issquare is a Function.                                               

  # 5 methods for generic function "issquare":   
  [1] issquare(::Union{Number, LinearAlgebra.UniformScaling}) in SciMLOperators at /Use
rs/vp/.julia/dev/SciMLOperators/src/interface.jl:197                                   
  [2] issquare(::SciMLOperators.AbstractSciMLScalarOperator) in SciMLOperators at /User
s/vp/.julia/dev/SciMLOperators/src/scalar.jl:29                                        
  [3] issquare(x::MatrixOperator, args...; kwargs...) in SciMLOperators at /Users/vp/.j
ulia/packages/Lazy/9Xnd3/src/macros.jl:297                                             
  [4] issquare(L) in SciMLOperators at /Users/vp/.julia/dev/SciMLOperators/src/interfac
e.jl:189                                                                               
  [5] issquare(A...) in SciMLOperators at /Users/vp/.julia/dev/SciMLOperators/src/inter
face.jl:198                                                                            

So it makes sense to just extend SciMLBase.issquare to act on LinearSolve.OperatorAssumptions. There's no reason they can't be separate. But since SciMLOperators will be compiled with LinearSolve anyway, it's just easier to extend the same function than to have two different functions with slightly different names doing the same thing.