Store linear invariants matrix in (Conservative)PDSProblem#163
Store linear invariants matrix in (Conservative)PDSProblem#163ranocha merged 13 commits intoNumericalMathematics:mainfrom
Conversation
Pull Request Test Coverage Report for Build 15341933359Details
💛 - Coveralls |
ranocha
left a comment
There was a problem hiding this comment.
I guess you are working with @SKopecz on this, correct? What are the plans for this feature - shall modified Patankar methods be able to use a single given linear invariant to rescale the setup to conserve it instead of the default linear invariant with weight vector
Depending on the goal, we may also consider using https://github.com/JuliaArrays/FillArrays.jl (Trues or Ones) as default for conservative problems.
|
@ranocha Yes, I'm working with @SKopecz on it. Our motivation was to implement the Sandu optimizer. Therefore, linear invariants are needed. At first, we hadn't considered MPRK methods in this context. However, your remark about the default values could make sense. Do you mean that for conservative problems, we set the linear invariants to one, but this can be changed if you have a problem that preserves additional linear invariants? |
The plan is to provide the matrix of linear invariants directly with the problems contained in To implement what you suggested, we would only need a vector. I think this should be implemented separately. |
|
I see, thanks! This makes sense. Would it make sense to rename the argument to |
|
I think so too. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
ranocha
left a comment
There was a problem hiding this comment.
Thanks! This looks already quite good to me.
| @@ -43,14 +43,15 @@ The functions `P` and `D` can be used either in the out-of-place form with signa | |||
| struct PDSProblem{iip} <: AbstractPDSProblem end | |||
There was a problem hiding this comment.
Since linear_invariants is a new feature, it should be documented at least in the docstring above. You can mention, for now, that it is an experimental feature so that we may change its behavior without having to release a breaking version of PositiveIntegrators.jl. If you do so, could you please also open an issue allowing us to track the status of this experimental feature?
There was a problem hiding this comment.
linear_invariants needs to be added at the beginning of the docstring as well.
PDSProblem(P, D, u0, tspan, p = NullParameters();
p_prototype = nothing,
analytic = nothing,
std_rhs = nothing,
linear_invariants = nothing)In addition, linear_invariants needs to be mentioned in the docstring of ConservativePDSProblem.
There was a problem hiding this comment.
Since
linear_invariantsis a new feature, it should be documented at least in the docstring above. You can mention, for now, that it is an experimental feature so that we may change its behavior without having to release a breaking version of PositiveIntegrators.jl.
It's not clear to me why the introduction of linear_invariants could be breaking. It's an optional feature and every code that ran in the past will also run in the future. Is it breaking because the PDSFunction and ConservativePDSFunction structs changed? But these structs are only used internally and are not documented.
There was a problem hiding this comment.
What Hendrik means is not that adding this feature is breaking, but rather that in the future it might be necessary or desirable to change this feature again (e.g. changing the API), which could break code then. Adding a note that this is experimental, allows us to do that without the need to care too much about creating a breaking release.
There was a problem hiding this comment.
linear_invariantsneeds to be added at the beginning of the docstring as well.PDSProblem(P, D, u0, tspan, p = NullParameters(); p_prototype = nothing, analytic = nothing, std_rhs = nothing, linear_invariants = nothing)In addition,
linear_invariantsneeds to be mentioned in the docstring ofConservativePDSProblem.
done
src/proddest.jl
Outdated
| the production-destruction representation of the ODE, will use this function | ||
| instead to compute the solution. If not specified, | ||
| a default implementation calling `P` and `D` is used. | ||
| -`linear_invariants`: Specifies the linear invariants of the problem as a `StaticArrays.SMatrix`. |
There was a problem hiding this comment.
linear_invariants must not be an SMatrix. I'd suggest the following:
`linear_invariants`: The rows of this matrix contain the linear invariants of the ODE. Certain solvers or callbacks require this matrix.The only solver/solution technique I can think of right now that requires the matrix of linear invariants is the Sandu projection. But I'd suggest to keep the docstring fairly general.
Furthermore, currently linear_invariants must not even be a matrix. Should we throw an error if it's not?
There was a problem hiding this comment.
I changed the text to the line above + note to experimental feature
The use of the Sandu projection in #162 requires a matrix containing the linear invariants. A
PDSFuntionorConservativePDSFunctionshould be able to store this matrix.lin_invariantsto the structsPDSFunctionandConservativePDSFunctionPDSProblemLibrary.jl