Add algorithm for N-body simulation - retry #4298
Conversation
![algorithms-keeper[bot]](https://web.archive.org/web/20211017081624im_/https://avatars.githubusercontent.com/in/86183?s=60&v=4){kind=small}
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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( |
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
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]: |
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
No doctest since this is an inner function.
You mean that because it is an inner function it cannot contain bugs?
It needs tests.
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.
I do not see the advantage of making this an inner function. Just make it a normal function with proper tests.
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.
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.
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.
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]: |
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
No doctest since this is an inner function.
Same.
other/n_body_simulation.py
Outdated
| @@ -0,0 +1,262 @@ | |||
| """ | |||
| In physics and astronomy, a gravitational N-body simulation is a simulation of a | |||
Should we have physics and/or (even better) astronomy directory so that we do not grow the other directory.
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.
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__": |
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) | ||
| ) |
Let's do this as a list comprehension.
other/n_body_simulation.py
Outdated
| >>> body.velocity_x | ||
| 1.0 |
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 |
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 |
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) | ||
| ) |
| 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 | |
| ] |
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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( |
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]: |
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]: |
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
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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.
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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.
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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.
|
Several additional doctests were added for the methods of |
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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, |
self should not be given a type hint. It is evaluated using dynamic dispatch during runtime. Please remove it from other places as well.
algorithms-keeper
bot
added
awaiting reviews
and removed
require tests
awaiting changes
labels
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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]: |
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]:
^
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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]: |
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]:
^
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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]: |
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]: |
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: |
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( |
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]: |
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: |
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: |
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
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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]: |
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]: |
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) |
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, |
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) |
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: | ||
| """ |
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: | ||
| """ |
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
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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]: |
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]: |
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: |
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( |
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]: |
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: |
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: |
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
Click here to look at the relevant links ⬇️
🔗 Relevant LinksRepository:
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]: |
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]: |
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: |
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( |
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]: |
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: |
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: |
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
I will create a follow up PR to add tests to examples.
|
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]>
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}.The text was updated successfully, but these errors were encountered: