-
Notifications
You must be signed in to change notification settings - Fork 99
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
Update rebound interface to Rebound release version 4.0.1 #1019
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM, nice!
It needs more extensive testing. Some features seem to work but on closer inspection require some settings to be changed, not all of which are available through the interface. |
Hm, so I was early. Should there be some kind of norm for this? When is a code integrated well enough that the PR adding it can be merged? That would then set some standards for reviews. All assuming that we're rich enough to be able to afford these kinds of things of course... |
I think in principle it can be merged (since it passes all current tests). However, since the new version of Rebound adds features, these need to be tested as well. |
That makes sense. |
Rebound should probably have a second worker built - one with, and one without OpenMP support. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 28 days if no further activity occurs. Thank you for your contributions. |
The Rebound version currently in AMUSE is quite old - this updates it to the latest release.
The current tests for the rebound interface still work, but should probably be extended.