Added Brownian motion class for manifolds #1938
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1938 +/- ##
==========================================
+ Coverage 91.23% 91.53% +0.31%
==========================================
Files 142 149 +7
Lines 12822 13573 +751
==========================================
+ Hits 11697 12423 +726
- Misses 1125 1150 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Nice @dralston78!
I've add a couple of commit changing the following (let me know if you disagree with something):
-
space.metricinstead of.metricbecause if space is equipped with another metric at a later stage, then it is taken into consideration by the code; otherwise it is ambiguous. (Same withspace.dim) -
I've removed the check on pytorch because someone may have closed form solutions for
christoffelsandcometric_matrix. if they don't have, then the existing error messages should be enough for users understand they need to change to autograd/pytorch -
I've removed the check
space.default_coords_type != "intrinsic"because unfortunately this flag is misused and sometimes some spaces have intrinsic coordinates, but have a string specifying which kind of coordinates it refers to (e.g. PoincareBall -> "ball"). It is related with Changedefault_coords_typeto boolean flagintrinsic=True#1725. I'll do such a change asap. -
I've removed the checks
not hasattr(space.metric, "christoffels")andnot hasattr(space.metric, "cometric_matrix")because these methods always exist, though usually raiseNotImplementedError. This is a design issues to which we haven't yet found a solution -
I've removed the check on
christoffelsandcometric_matrixshapes because I don't want to pay the price of running both once only for shape checking. -
for the reasons above, we are not doing checks and assume people know when they can use
BrownianMotion- after fixingintrinsic, we can to aninstantiationtime check that will be equivalent to all the current tests -
I've added a name to the reference in the docstring, so that we can reference it in the text in case we need.
-
I've updated
sample_pathdocstrings. -
I've collected paths in a list and then used stack so that we avoid instantiating an array of zeros
-
I've vectorized
sample_path, i.e. allowed it to receive multiple points, by addingellipsestogs.einsumand ensuring we sample independently for each point.
Regarding tests:
- I've done small changes, but no update in the behavior (they're very nice!)
|
|
||
| @autograd_and_torch_only | ||
| @pytest.mark.slow | ||
| class TestBrownianMotion(TestCase): |
There was a problem hiding this comment.
Unit-tests should be unit, ie we should have at least one unit test per function.
Missing test_sample_path and missing test__step
There was a problem hiding this comment.
Hi @ninamiolane, thank you for this clarification! With regards to this comment, would the ideal structure be to have a test class TestBrownianMotion, and then have this class have two methods test_sample_path and test__step?
There was a problem hiding this comment.
I think tests are already very good here, @ninamiolane. Two reasons: 1) this computation is relatively slow, so it is better to not add too many redundant tests; 2) there's randomness involved (we are sampling from a normal), which mean we cannot do a "normal test" (the checks done by @dralston78 are pretty clever).
|
@dralston78, with #1943 merged, we can now check for |
Merging updates from upstream repository.
…tats into brownian_motion Pulled updates to the original pull request.
Great! Would you like me to add a private method |
|
@dralston78, adding back |
…ifold is parameterized in terms of intrinsic coordinates.
|
@ninamiolane, this looks ready to me. Can we merge? |
Checklist
Description
I implemented a class to generate a realization of a Brownian motion walk on a manifold in intrinsic coordinates. While Brownian motion on a manifold is often defined as a Stratonovich stochastic differential equation (SDE), I have used the converted form to an Ito SDE in order to use the Euler–Maruyama method. I added a unit test to verify the axioms of Brownian motion in Euclidean space.
Issue
Additional context
This class requires the pytorch backend.
The method to generate a path requires two conditions on the underlying manifold. First, the manifold must be described in terms of its intrinsic coordinates. Second, the manifold must be equipped with a metric that has a
cometric_matrixmethod and achristoffelsmethod.I proposed this idea to Professor Miolane at the end of her seminar class earlier this year, and she encouraged me to create an implementation. However, as I don't have a background in software engineering, I wouldn't be surprised if there are some bad practices in my code/tests, or other flaws. I'm looking forward to getting your feedback and learning more!