Repository navigation
Add support for multidimensional IDEs - #35
Conversation
JoshKarpel
left a comment
There was a problem hiding this comment.
This is awesome! Thank you so much for looking into this. Looks like the changes weren't too painful.
re: the unexpectedly-complex-valued-IDE check, I suspect that what's happening is that, because the inputs are now arrays, numpy is throwing an actual exception instead of a warning: TypeError: can't convert complex to float (buried in the middle of the failing step of https://github.com/JoshKarpel/idesolver/pull/35/checks?check_run_id=1413226346). I think you can make it behave by changing the except np.ComplexWarning block to
except (np.ComplexWarning, TypeError) as e:
raise exceptions.UnexpectedlyComplexValuedIDE(
"Detected complex-valued IDE. Make sure to pass y_0 as a complex number."
) from e| if self.store_intermediate: | ||
| for j in range(len(self.y_intermediate)): | ||
| self.y_intermediate[j] = self.y_intermediate[0] |
There was a problem hiding this comment.
It's not obvious to me that this is right - seems like it should be something like self.y_intermediate = [y[0] for y in self.y_intermediate]. Could you add a new test that asserts that the structure of y_intermediate ends up being correct (a list of numbers if ndim == 1, else a list of numpy arrays, I think).
There was a problem hiding this comment.
If we can also use zeros_like in other places, we might be able to avoid storing self.ndim at all.
There was a problem hiding this comment.
It was false indeed, I forgot the index j:
self.y_intermediate[j] = self.y_intermediate[j][0]
But your solution is more Pythonic.
| y0=np.array([self.y_0]), | ||
| y0=self.y_0, |
There was a problem hiding this comment.
In hindsight, this makes it rather obvious that it wasn't too far away from working!
| ndmin=1, | ||
| copy=False) | ||
|
|
||
| result = np.zeros(self.ndim, dtype=type(y)) |
There was a problem hiding this comment.
Actually zeros_like here make the tests fail, I think when self.y_0 is an integer.
We can have result = np.zeros_like(self.y_0, dtype=type(y)) instead.
| k = lambda x, s: 1 | ||
| if f is None: | ||
| f = lambda y: 0 | ||
| f = lambda y: np.zeros(self.ndim) |
There was a problem hiding this comment.
Maybe np.zeros_like(self.y_0)?
|
|
||
| if c is None: | ||
| c = lambda x, y: 0 | ||
| c = lambda x, y: np.zeros(self.ndim) |
There was a problem hiding this comment.
Maybe np.zeros_like(self.y_0)?
- Improve style - Array coercion for all the input functions - Catch error for complex valued functions when y_0 is real, - Correct conversion of y_intermediate for the scalar case - Add test for the shape of y_intermediate
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
====================================
Coverage 97% 98%
====================================
Files 11 11
Lines 286 305 +19
Branches 42 55 +13
====================================
+ Hits 280 299 +19
Misses 3 3
Partials 3 3
|
|
@nbrucy I think I improved the dtype handling a little, in exchange for slightly less elegance. I also fixed a separate problem with coverage uploads that was breaking CI. CI is green and I like the changes, so let me know if you're good with the dtype handling and then I can merge. |
|
It seems indeed to be more robust in case of inconsistencies between the types returned by the input functions. |
|
Thank you so much @nbrucy ! This is a fantastic improvement. I'll try to cut a new release ASAP. |
This PR resolves #28 by adding support to multidimensional IDE.
of which the analytical solution is (sin(x), cos(x))
It passes all the tests except one :
test_raise_exception_if_unexpectedly_complex.I don't know how bad it is since it is expected to fail but maybe some extra checks are needed.