Closed arjunsavel closed 3 years ago
This is so awesome! Thanks and let me know as things progress.
@dfm ready for review! Not sure why the Windows test fails, but could be related to #92 (there were intermittent Windows failures in previous commits). Happy to write more tests/documentation if this implementation is fine!
Sorry about the slow response! Just catching up on things over here.
This looks really excellent and I'm happy to merge.
One question: would you be willing to hack this a bit to work even if reboundx isn't installed? Perhaps there could be an import error that gets thrown if you try to include relativity, but don't have reboundx installed, otherwise, it works fine with just rebound?
Thanks again for the contribution!!
No worries, and I'll be sure to implement the import error!
Sorry that this took so long! I've added the error handling if reboundx isn't installed. The only test that should fail now is test_full_adapt_sampling
, which I think has been fixed on main.
@arjunsavel: Some how I totally missed your update here - I'm so sorry!! I've just fixed some merge conflicts and once the tests pass, I'll merge. Thanks and again I'm sorry for being so slow to get back to this!
Take a look at #133 for an updated version of this PR.
Not yet ready to merge.
TODO:
reboundx
isn't installedMotivation Looking to address #54; namely, wanting to add relativistic effects to the code to model more massive systems, e.g. binary stars, high-mass planets orbiting close to high-mass stars, systems near black holes, etc.
I plan on approaching this in two ways: firstly by introducing the change to orbits, and secondly by adding gravitational redshifting (which @ericagol was referencing in the original issue). I've made headway on the former, but the latter might warrant its own PR.
Description
Currently, I've made an integration of reboundx into the Python implementation of ReboundOp. If this style of integration is workable, I'll work on a similar PR to
rebound-pymc3
. I might want to create a new orbit that inherits from ReboundOrbit, given that it has a new citation (which I've added to the citation file) — but that might be the only real difference, so not sure if an entirely new orbit is warranted. A demo showing the instantiation of a gr orbit is depicted below. Other forces ("gr_full", "gr_potential") can be passed as well, but the below images only demonstrate the "gr" force.Testing The local tests run as expected (155 passed, 12 skipped, 1 xfailed), and I plan on writing a few more tests for this feature:
There is a time penalty for using the GR model — for the ''gr" force, my fiducial model sees a runtime increase of ~60% (29.2 +/- 2.02s to 47.3 +/-1.76s on my system).
Relevant screenshots
I've also attached some images that demonstrate the expected differences.
The above image shows how users turn on relativistic effects, simply by using binary flags. A PR to the C bindings for ReboundOp would make the extra ReboundOp argument unnecessary.
For my fiducial model (first image), we see the accumulated difference in x-velocity of the modeled planet over the course of these roughly 3,500 days.