-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
custom linear solve function #112
Conversation
suggestion from Daniel Wennberg on DiffEq-bridged from Julia Slack if applicable(ldiv!, A, b)
[ldiv! code]
else
[no-ldiv! code]
end |
julia> using LinearSolve, LinearAlgebra;prob=LinearProblem(Diagonal(rand(4)),rand(4);u0=rand(4)); alg=FunctionCall(ldiv!, (:u, :A, :b)); cache=SciMLBase.init(prob, alg); solve(cache)
u: 4-element Vector{Float64}:
0.07077439381665458
1.22568741027046
1.2092496048689403
1.3612307917964956
alg works. now need to figure out |
For the diagonal case, it would probably be best just keep the standard format via using julia> A = Diagonal(rand(2))
2×2 Diagonal{Float64, Vector{Float64}}:
0.241201 ⋅
⋅ 0.0374477
julia> factorize(A)
2×2 Diagonal{Float64, Vector{Float64}}:
0.241201 ⋅
⋅ 0.0374477 which should be the current default? |
src/default.jl
Outdated
if A isa DiffEqArrayOperator | ||
A = A.A | ||
end | ||
|
||
if applicable(ldiv!, A, u) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this check static?
i don't understand what's causing type instability so ive pushed a few options to ive also reverted all changes to |
src/function_call.jl
Outdated
|
||
## | ||
|
||
""" apply ldiv!(A, u) """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be type stable, correct @ChrisRackauckas ?
The more sane way of doing this is to just impose a function syntax. For example, impose that it's always |
CI |
weird tests didn't run. closing/opening again |
@ChrisRackauckas can you take a look, and run CI? tests aren't running for me |
Needs documentation. |
Otherwise I think this looks good to go. |
Rebase this onto master. |
Codecov Report
@@ Coverage Diff @@
## main #112 +/- ##
==========================================
+ Coverage 58.15% 59.20% +1.05%
==========================================
Files 10 11 +1
Lines 595 603 +8
==========================================
+ Hits 346 357 +11
+ Misses 249 246 -3
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
rebased, tests passing |
merge, @ChrisRackauckas ? |
Needs docs. |
Pull Request Test Coverage Report for Build 2105740759Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
docs added |
src/solve_function.jl
Outdated
end | ||
|
||
Base.@kwdef struct LinearSolveFunction{F} <: AbstractSolveFunction | ||
solve_func::F = DEFAULT_LINEAR_SOLVE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there should be a default here. If they use LinearSolveFunction, they are explicitly opting out of using the default, so it would be unsafe to just make it fallback. Instead, it should error and say hey, didn't you wish to provide a function here?
fixes #110would be good to have