-
Notifications
You must be signed in to change notification settings - Fork 4
Allow d(x) and k(x, s) to be matrices
#60
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
======================================
Coverage 98.3% 98.4%
======================================
Files 11 11
Lines 308 316 +8
Branches 55 57 +2
======================================
+ Hits 303 311 +8
Misses 2 2
Partials 3 3
Continue to review full report at Codecov.
|
|
@nbrucy since you added the original multi-dimensional IDE code, would you mind taking a look at this and letting me know if anything looks off? Or if you know any tricks for handling the scalar-vs-matrix multiplication switch more smoothly... |
idesolver/idesolver.py
Outdated
| return self.c(x, interpolated_y(x)) + (self.d(x) * integral(x)) | ||
| c = self.c(x, interpolated_y(x)) | ||
| d = self.d(x) | ||
| if d.size == 1: |
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 suggest to replace d.size by d.ndim
| lambda x: [np.sin(x), np.cos(x)], | ||
| ), | ||
| ( # equivalent to previous case, but with k(x, s) as a matrix instead of a scalar | ||
| ( # equivalent to previous case, but with d(x) and k(x, s) as matrices |
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 can add the following test if d and k are vector valued.
( # equivalent to previous case, but with d(x) and k(x, s) as vectors
IDESolver(
x=np.linspace(0, 7, 100),
y_0=[0, 1],
c=lambda x, y: [0.5 * (y[1] + 1), -0.5 * y[0]],
d=lambda x: [-0.5, -0.5],
k=lambda x, s: [1, 1],
f=lambda y: y,
lower_bound=lambda x: 0,
upper_bound=lambda x: x,
),
lambda x: [np.sin(x), np.cos(x)],
),
|
Hello! I don’t see better way of switching between scalar and matrix multiplications but have you considered the case when d and k are vector-valued? I think this test case would work before the PR and won't now:
I guess a simple solution would be do the test on |
idesolver/idesolver.py
Outdated
| k = self.k(x, s) | ||
| f = self.f(interpolated_y(s)) | ||
|
|
||
| if k.size == 1: |
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 suggest to replace k.size by k.ndim
Thank you so much! I've implemented your suggestions. |
|
Nice! Looks goods for me now. |
In #35 , support was added for multi-dimensional IDEs. However, it did not account for the case where
d(x)andk(x,s)might mix the dimensions, only thatc(y, x)might. That case mostly-silently worked because of how numpy handles addition broadcasting; multiplication broadcasting is more complex and it seems like the simplest solution is to hackily switch on the array size. Not really sure whether this is the "proper" way of doing things, but it passes the test case I cooked up for it. I suppose we could forced(x)andk(x, s)to be matrices ify_0(x)is multi-dimensional, but that would be annoying to write out in many cases.