Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Does this mean that plain
Array
s ofSVector
s will not be supported anymore and we have to switch toVectorOfArray
?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.
See SciML/SciMLBase.jl#561. Basically I am trying to get things to a well-defined interface. This little bit sticks out as sore area of DiffEq where we don't throw any errors about arrays of arrays since 10 out of like 300 methods supports arrays of arrays. But we should be able to support such types of targets more broadly without overhead by just using a small wrapper that makes everything conform to the interface. If this is allowed, then we can throw informative errors as to what's going on and we can vastly clean up a lot of the code to not require hacks for specific methods to avoid assumptions about the lack of Number eltype. Since it's an undocumented bit only used downstream by you and @Datseris, it's a small discussion to be had with many good consequences to most users, and would make things like transitioning a code to implicit methods more readily available since the Vector of Vector form will likely never be able to support linear algebra appropriately.
As you can see from the PR, there is a keyword argument being made to bypass the interface erroring, but I hope that is only used during development and is not a crutch that is actually relied on for these kinds of tasks given that there should be zero overhead to using a more structured and interface-compliant form.
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.
We use arrays of
SVector
s at least for some solvers in Trixi.jl with explicit Runge-Kutta methods. If OrdinaryDiffEq.jl transitions to another interface requirement, it would be great to get a breaking release describing this change.We would also need more interfaces such as
resize!
andpointer
forVectorOfArray
.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.
Which PR do you refer to? I could only see changes to the tests here and new docs at SciML/SciMLBase.jl#561
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'm not sure undocumented side pieces really constitute breaking. We are trying to impose such interfaces so that there is a strong sense of what is actually breaking.
Add suggestions in the interface PR and I'll happily accept that.
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.
In BVDiffEq we were using a
AbstractArray{<:AbstractArray}
for specifying the initial guess on a mesh. We can upperbound SciMLBase there and cut a breaking release.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 bounding SciMLBase compat enough to address the impact of this change?
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.
Not SciMLBase my bad. DiffEqBase needs a upperbound. See SciML/BoundaryValueDiffEq.jl#152
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.
BVDiffEq we need to specialize there because that's a legitimate use case, that's quite different.
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.
How do you plan to do that? We may use it as inspiration for us in Trixi.jl.