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

removing particles changes existing variables #418

Closed
Findus23 opened this issue Mar 30, 2020 · 2 comments · Fixed by #419
Closed

removing particles changes existing variables #418

Findus23 opened this issue Mar 30, 2020 · 2 comments · Fixed by #419

Comments

@Findus23
Copy link
Contributor

Hi,

Thanks for the great project. I'm quite a beginner to n-body integration, but nevertheless rebound is easy to use.

This is not technical a bug, as it is working correctly, but not as one would expect and I think this could trip up beginners and might warrant a note in the docs.

Minimal example:

from rebound import Simulation, Particle

sim = Simulation()
sim.units = ('yr', 'AU', 'kg')
sim.add(m=100, hash=0)
# let's add planets with easy to guess hashes
for i in range(1, 10):
    sim.add(a=i, hash=i, m=1e10)

# now let's pick two random particles (e.g. they were in a collision together like in CloseEncounters.ipynb)
cp1: Particle = sim.particles[3]
print(cp1.hash.value)  # returns '3' as expected
cp2: Particle = sim.particles[7]
print(cp2.hash.value)  # returns '7' as expected

# let's say we we want to remove those two particles from the simulation after the collision
print([p.hash.value for p in sim.particles])
sim.remove(hash=cp1.hash)
print([p.hash.value for p in sim.particles])  # now only [0, 1, 2, 4, 5, 6, 7, 8, 9] are left as 3 got removed
sim.remove(hash=cp2.hash)
print([p.hash.value for p in sim.particles])
# now one would expect 7 to also be gone
# but it seems like cp2 only points to the index in the particle list, so it was now in fact the particle with the hash 8
# as a result 8 is gone, but 7 remains
# [0, 1, 2, 4, 5, 6, 7, 9]

The solution is quite easy once one knows what happens:

cp1_hash = cp1.hash
cp2_hash = cp2.hash

sim.remove(hash=cp1_hash)
sim.remove(hash=cp2_hash)

But finding out that this happened wasn't obvious until I saw the same particle collide multiple times.

@hannorein
Copy link
Owner

Hi Lukas,

Thanks for opening an issue about this. So just to confirm, I think what you were confused with is that sim.particles returns a pointer, not a copy of the particle array. As you correctly concluded, this is not a bug, but we've made this decision on purpose. If it returned a copy, you could never do something like sim.particles[4].m=6 because it would only change the copy, not the actual particle in the simulation. We mention this in multiple locations in the documentation, but maybe not prominently enough. If you can think of a specific spot where we should mention it, let me know (or even better, send a pull request!).

Thanks again,
Hanno

@Findus23
Copy link
Contributor Author

Hi, you are right. This is what confused me. I expected the individual entries in the list to be pointers, but not the whole list ( or rather the python variable to be a reference to the single particle and not the position in the list).

To be fair, the documentation of sim.particles that I didn't read beforehand says this quite clearly.

rebound/rebound/simulation.py

Lines 1303 to 1305 in 127f7c9

The Particles object uses pointers and thus the contents update
as the simulation progresses. Note that the pointers could change,
for example when a particle is added or removed from the simulation.

I'd recommend also mentioning this in the notebook and will create a PR

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 a pull request may close this issue.

2 participants