Add algorithm for N-body simulation - retry #4298
Conversation
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
other/n_body_simulation.py
Outdated
| body.update_position(delta_time * self.time_factor) | ||
|
|
||
|
|
||
| def plot( |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file other/n_body_simulation.py, please provide doctest for the function plot
There was a problem hiding this comment.
No doctest provided since this function does not have a return value, it just plots the result of the algorithm.
other/n_body_simulation.py
Outdated
| ) | ||
|
|
||
| # Function called once at the start of the animation | ||
| def init() -> list[patches.Circle]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file other/n_body_simulation.py, please provide doctest for the function init
There was a problem hiding this comment.
No doctest since this is an inner function.
There was a problem hiding this comment.
You mean that because it is an inner function it cannot contain bugs?
It needs tests.
There was a problem hiding this comment.
I'm not sure that it's possible to write a doctest for an inner function, for example, see https://stackoverflow.com/questions/2136910/can-i-unit-test-an-inner-function-in-python or https://bugs.python.org/issue1650090 . I could define the 2 functions outside the plot-function and add doctests. But that would be a little unintuitive, since they are just part of the plotting and not of the algorithm proper. But my experience with doctest is limited so I'm not sure what the best approach here is.
There was a problem hiding this comment.
I do not see the advantage of making this an inner function. Just make it a normal function with proper tests.
There was a problem hiding this comment.
I ran into some problems while trying to define init & update outside the scope of plot, since these functions make use of patches, which is defined inside plot. Something like this seems to be the normal procedure for using matplotlib animation, for example, see line in https://jakevdp.github.io/blog/2012/08/18/matplotlib-animation-tutorial/ .
I don't think that we can pass patches as an argument to these functions since animation.FuncAnimation uses them as callbacks. It might be possible to solve this problem through currying, but that would make it needlessly complicated and may defy the attempt to avoid using inner functions. But maybe there is an easier solution that I'm missing.
There was a problem hiding this comment.
Another approach would be to define patches (& possibly other local variables needed) globally and then import them into the functions using the global-keyword. It should work but it's not pretty.
There was a problem hiding this comment.
If you want to keep the inner functions then the outer function should be heavily tested. Let's put all calculation in a separate function from the plotting function.
Make the function that plots as small as possible... That is, make one well-tested function that calculates the results and another smaller, untested function that performs no calculations but merely plots the results.
There was a problem hiding this comment.
That sounds like a good solution.
other/n_body_simulation.py
Outdated
| return patches | ||
|
|
||
| # Function called at each step of the animation | ||
| def update(frame: int) -> list[patches.Circle]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file other/n_body_simulation.py, please provide doctest for the function update
There was a problem hiding this comment.
No doctest since this is an inner function.
other/n_body_simulation.py
Outdated
| @@ -0,0 +1,262 @@ | |||
| """ | |||
| In physics and astronomy, a gravitational N-body simulation is a simulation of a | |||
There was a problem hiding this comment.
Should we have physics and/or (even better) astronomy directory so that we do not grow the other directory.
There was a problem hiding this comment.
If we make it physics, we could also include the files from electronics and remove one root-directory. But in any case, I think physics is the better choice since this is not specifically about celestial objects.
There was a problem hiding this comment.
Let's add physics with this algorithm but let's not migrate electronics into it.
other/n_body_simulation.py
Outdated
| plt.show() | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Please break this long collection of code into three separate functions.
| if __name__ == "__main__": | |
| if __name__ == "__main__": | |
| example_1() | |
| example_2() | |
| example_3() |
other/n_body_simulation.py
Outdated
| patches = [] | ||
| for body in body_system.bodies: | ||
| patches.append( | ||
| plt.Circle((body.position_x, body.position_y), body.size, fc=body.color) | ||
| ) |
There was a problem hiding this comment.
Let's do this as a list comprehension.
other/n_body_simulation.py
Outdated
| >>> body.velocity_x | ||
| 1.0 |
There was a problem hiding this comment.
Please also test body.velocity_y
other/n_body_simulation.py
Outdated
| >>> body = Body(0.,0.,1.,0.) | ||
| >>> body.update_position(1.) | ||
| >>> body.position_x | ||
| 1.0 |
There was a problem hiding this comment.
Please also test body.position_y
other/n_body_simulation.py
Outdated
|
|
||
| >>> body_system = BodySystem([Body(0,0,0,0), Body(10,0,0,0)]) | ||
| >>> body_system.update_system(1) | ||
| >>> body_system.bodies[0].position_x |
There was a problem hiding this comment.
Please test other variables as well.
other/n_body_simulation.py
Outdated
| patches = [] | ||
| for body in body_system.bodies: | ||
| patches.append( | ||
| plt.Circle((body.position_x, body.position_y), body.size, fc=body.color) | ||
| ) |
There was a problem hiding this comment.
| patches = [] | |
| for body in body_system.bodies: | |
| patches.append( | |
| plt.Circle((body.position_x, body.position_y), body.size, fc=body.color) | |
| ) | |
| patches = [ | |
| plt.Circle((body.position_x, body.position_y), body.size, fc=body.color) | |
| for body in body_system.bodies | |
| ] |
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
| body.update_position(delta_time * self.time_factor) | ||
|
|
||
|
|
||
| def plot( |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function plot
physics/n_body_simulation.py
Outdated
| ) | ||
|
|
||
| # Function called once at the start of the animation | ||
| def init() -> list[patches.Circle]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function init
physics/n_body_simulation.py
Outdated
| return patches | ||
|
|
||
| # Function called at each step of the animation | ||
| def update(frame: int) -> list[patches.Circle]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function update
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
ghost
added
tests are failing
|
Several additional doctests were added for the methods of |
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
physics/n_body_simulation.py
Outdated
|
|
||
| class Body: | ||
| def __init__( | ||
| self: Body, |
There was a problem hiding this comment.
self should not be given a type hint. It is evaluated using dynamic dispatch during runtime. Please remove it from other places as well.
ghost
added
awaiting changes
ghost
added
awaiting reviews
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
physics/n_body_simulation.py
Outdated
| self.color = color | ||
|
|
||
| @property | ||
| def position(self) tuple[float, float]: |
There was a problem hiding this comment.
An error occured while parsing the file: physics/n_body_simulation.py
Traceback (most recent call last):
File "/app/.heroku/python/lib/python3.8/site-packages/libcst/_parser/base_parser.py", line 152, in _add_token
plan = stack[-1].dfa.transitions[transition]
KeyError: TokenType(NAME)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/app/algorithms_keeper/parser/python_parser.py", line 145, in parse
reports = lint_file(
libcst._exceptions.ParserSyntaxError: Syntax Error @ 47:24.
Incomplete input. Encountered 'tuple', but expected '->', or ':'.
def position(self) tuple[float, float]:
^
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
physics/n_body_simulation.py
Outdated
| return self.position_x, self.position_y | ||
|
|
||
| @property | ||
| def velocity(self) tuple[float, float]: |
There was a problem hiding this comment.
An error occured while parsing the file: physics/n_body_simulation.py
Traceback (most recent call last):
File "/app/.heroku/python/lib/python3.8/site-packages/libcst/_parser/base_parser.py", line 152, in _add_token
plan = stack[-1].dfa.transitions[transition]
KeyError: TokenType(NAME)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/app/algorithms_keeper/parser/python_parser.py", line 145, in parse
reports = lint_file(
libcst._exceptions.ParserSyntaxError: Syntax Error @ 51:24.
Incomplete input. Encountered 'tuple', but expected '->', or ':'.
def velocity(self) tuple[float, float]:
^
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
| self.color = color | ||
|
|
||
| @property | ||
| def position(self) -> tuple[float, float]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function position
| return self.position_x, self.position_y | ||
|
|
||
| @property | ||
| def velocity(self) -> tuple[float, float]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function velocity
physics/n_body_simulation.py
Outdated
| self.time_factor = time_factor | ||
| self.softening_factor = softening_factor | ||
|
|
||
| def __len__() -> int: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function __len__
| patch.center = (body.position_x, body.position_y) | ||
|
|
||
|
|
||
| def plot( |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function plot
| ax.add_patch(patch) | ||
|
|
||
| # Function called at each step of the animation | ||
| def update(frame: int) -> list[plt.Circle]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function update
| return BodySystem(bodies1, time_factor=3) | ||
|
|
||
|
|
||
| def example_2() -> BodySystem: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function example_2
| return BodySystem([earth, moon], gravitation_constant, time_factor=1000000) | ||
|
|
||
|
|
||
| def example_3() -> BodySystem: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function example_3
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
| self.color = color | ||
|
|
||
| @property | ||
| def position(self) -> tuple[float, float]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function position
| return self.position_x, self.position_y | ||
|
|
||
| @property | ||
| def velocity(self) -> tuple[float, float]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function velocity
| self.softening_factor = softening_factor | ||
|
|
||
| def __len__() -> int: | ||
| return len(self.bodies) |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function __len__
|
|
||
|
|
||
| def plot( | ||
| title: str, |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function plot
|
|
||
| # Function called at each step of the animation | ||
| def update(frame: int) -> list[plt.Circle]: | ||
| update_step(body_system, DELTA_TIME, patches) |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function update
|
|
||
|
|
||
| def example_2() -> BodySystem: | ||
| """ |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function example_2
|
|
||
|
|
||
| def example_3() -> BodySystem: | ||
| """ |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function example_3
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
| self.color = color | ||
|
|
||
| @property | ||
| def position(self) -> tuple[float, float]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function position
| return self.position_x, self.position_y | ||
|
|
||
| @property | ||
| def velocity(self) -> tuple[float, float]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function velocity
| self.time_factor = time_factor | ||
| self.softening_factor = softening_factor | ||
|
|
||
| def __len__(self) -> int: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function __len__
| patch.center = (body.position_x, body.position_y) | ||
|
|
||
|
|
||
| def plot( |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function plot
| ax.add_patch(patch) | ||
|
|
||
| # Function called at each step of the animation | ||
| def update(frame: int) -> list[plt.Circle]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function update
| return BodySystem(bodies1, time_factor=3) | ||
|
|
||
|
|
||
| def example_2() -> BodySystem: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function example_2
| return BodySystem([earth, moon], gravitation_constant, time_factor=1000000) | ||
|
|
||
|
|
||
| def example_3() -> BodySystem: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function example_3
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
| self.color = color | ||
|
|
||
| @property | ||
| def position(self) -> tuple[float, float]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function position
| return self.position_x, self.position_y | ||
|
|
||
| @property | ||
| def velocity(self) -> tuple[float, float]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function velocity
| self.time_factor = time_factor | ||
| self.softening_factor = softening_factor | ||
|
|
||
| def __len__(self) -> int: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function __len__
| patch.center = (body.position_x, body.position_y) | ||
|
|
||
|
|
||
| def plot( |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function plot
| ax.add_patch(patch) | ||
|
|
||
| # Function called at each step of the animation | ||
| def update(frame: int) -> list[plt.Circle]: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function update
| return BodySystem(bodies1, time_factor=3) | ||
|
|
||
|
|
||
| def example_2() -> BodySystem: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function example_2
| return BodySystem([earth, moon], gravitation_constant, time_factor=1000000) | ||
|
|
||
|
|
||
| def example_3() -> BodySystem: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file physics/n_body_simulation.py, please provide doctest for the function example_3
|
Thanks for helping with all the changes. It's definitely more elegantly handled this way. |
* add n_body_simulation.py
* updating DIRECTORY.md
* Rename other/n_body_simulation.py to physics/n_body_simulation.py
* updating DIRECTORY.md
* Update build.yml
* refactor examples & add doctests
* removed type-hints from self-parameter
* Apply suggestions from code review
* Update physics/n_body_simulation.py
* Update physics/n_body_simulation.py
* Update physics/n_body_simulation.py
* Don't forget self
* Fix velocity
Co-authored-by: github-actions <${GITHUB_ACTOR}@users.noreply.github.com>
Co-authored-by: Dhruv Manilawala <[email protected]>
Co-authored-by: Christian Clauss <[email protected]>
* add n_body_simulation.py
* updating DIRECTORY.md
* Rename other/n_body_simulation.py to physics/n_body_simulation.py
* updating DIRECTORY.md
* Update build.yml
* refactor examples & add doctests
* removed type-hints from self-parameter
* Apply suggestions from code review
* Update physics/n_body_simulation.py
* Update physics/n_body_simulation.py
* Update physics/n_body_simulation.py
* Don't forget self
* Fix velocity
Co-authored-by: github-actions <${GITHUB_ACTOR}@users.noreply.github.com>
Co-authored-by: Dhruv Manilawala <[email protected]>
Co-authored-by: Christian Clauss <[email protected]>
Describe your change:
New pull request to replace #4245 since the old pull request ran into a branch conflict after updating. The changes to #4245 include better comments and more descriptive names.
Checklist:
Fixes: #{$ISSUE_NO}.