Skip to content
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

Add relativity #98

Merged
merged 115 commits into from
Jan 4, 2021
Merged

Add relativity #98

merged 115 commits into from
Jan 4, 2021

Conversation

arjunsavel
Copy link
Contributor

@arjunsavel arjunsavel commented May 28, 2020

Not yet ready to merge.

TODO:

  • Throw import error if reboundx isn't installed

Motivation
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:

  • testing that there be no velocity discrepancy between models if all GR flags are set to 0
  • similarly, that there be no difference if GR is turned on but for very low masses, and
  • that there should be a large (pre-calculated) deviation if GR is turned on for a very large mass.

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.

instantiation

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.

short_time

longer_time

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.

@dfm
Copy link
Member

dfm commented May 28, 2020

This is so awesome! Thanks and let me know as things progress.

@arjunsavel arjunsavel marked this pull request as ready for review June 12, 2020 16:38
@arjunsavel
Copy link
Contributor Author

@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!

@dfm dfm changed the base branch from master to main July 21, 2020 15:18
@dfm
Copy link
Member

dfm commented Jul 21, 2020

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!!

Omega=1.0,
incl=0.25 * np.pi,
m_planet=m_planet,
ReboundOp=test_rebound_op,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of having this here? Is this so that rebound_pymc3 doesn't get used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! I took inspiration from the theano_ops testing.

@arjunsavel
Copy link
Contributor Author

No worries, and I'll be sure to implement the import error!

@arjunsavel arjunsavel marked this pull request as draft July 27, 2020 01:18
@arjunsavel arjunsavel marked this pull request as ready for review October 11, 2020 18:54
@arjunsavel
Copy link
Contributor Author

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.

emilygilbert and others added 13 commits October 19, 2020 14:46
* Update Python and PyMC3 versions

* No Python 3.9 on conda yet

* Combine coverage
…t the --recursive flag (exoplanet-dev#119)

* added --recursive flag to the install from source documentation

* added info for what to do if you forgot the --recursive flag when cloning

* added info for what to do if you forgot the --recursive flag when cloning
* Update setup.py

* use conda

* update conda action version

* test on PyMC3 3.10

* install starry with no-deps

* new theano compat

* testing compat

* fixing testval in unit vector test
@dfm
Copy link
Member

dfm commented Dec 31, 2020

@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!

@dfm dfm mentioned this pull request Dec 31, 2020
@dfm
Copy link
Member

dfm commented Dec 31, 2020

Take a look at #133 for an updated version of this PR.

@dfm dfm merged commit 4556689 into exoplanet-dev:main Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants