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

discussion: remove id exposure from interface #3992

Open
KaiSzuttor opened this issue Nov 13, 2020 · 11 comments
Open

discussion: remove id exposure from interface #3992

KaiSzuttor opened this issue Nov 13, 2020 · 11 comments

Comments

@KaiSzuttor
Copy link
Member

No description provided.

@KaiSzuttor KaiSzuttor changed the title discussion: remove id exposurefrom interface discussion: remove id exposure from interface Nov 13, 2020
@KaiSzuttor
Copy link
Member Author

from the user perspective the system state should not change if two particles are exchanged. the particle id is an implementation detail. this would also remove the odd slice index to id coupling which does not make sense to me

@jngrad
Copy link
Member

jngrad commented Nov 16, 2020

import numpy as np
import espressomd
system = espressomd.System(box_l=3*[10])
for i in range(3):
    system.part.add(pos=np.random.random(3) * system.box_l)

system.part[1].remove()
system.part[:].pos  # returns 2 elements
system.part[1].pos  # RuntimeError: Particle node for id 1 not found

@KaiSzuttor
Copy link
Member Author

import numpy as np
import espressomd
system = espressomd.System(box_l=3*[10])
for i in range(3):
    system.part.add(pos=np.random.random(3) * system.box_l)

system.part[1].remove()
system.part[:].pos  # returns 2 elements
system.part[1].pos  # RuntimeError: Particle node for id 1 not found

if somebody knows python, this is a very surprising behavior since there is no slice with gaps in the python world. i'd really prefer not to hold onto this slicing syntax.

@fweik
Copy link
Contributor

fweik commented Nov 16, 2020

While I agree with you in principal, I think from a practical point of view I think this is unwise: It will break most user scripts, without there being an obvious reason for the break. What is your cost/benefit analysis for this? I also think that there are more
serious problems in the particle interface that should be addressed first, e.g. that the code is not reusable for particles that are not in the cell system, and the implementation of the slices seems rather incidental (pythonic?) to me.

(This think this also smells of the Potemkin Pattern)

@KaiSzuttor
Copy link
Member Author

I obviouly did not do a complete analysis, that's why I wanted to have a lively discussion in this issue.

Generally I think that chamging this slicing interface could lead to more readable simulation scripts. I'm thinking of giving groups of filtered particles a name for further reuse, e.g..
But yes, breaking interface changes should not be done, if we do bot have a major benefit overall.

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Nov 17, 2020 via email

@fweik
Copy link
Contributor

fweik commented Nov 17, 2020

On a general note, I have come to think that the Potemkin Pattern is a much bigger design issue than we used to think. The problem with emulating a different API in the interface than in the core is that this always encodes semantic information into the python interface (as opposed to merely syntactic information), and as such is always problematic. If you think about this, the purpose of such a construction is actually coupling...

@pkreissl
Copy link
Contributor

As already mentioned in PR #4058, there are for now still some places, especially in the espressomd.observables, where cannot be omitted, such as ParticlePositions.

@KaiSzuttor
Copy link
Member Author

of course we would need to have typebased observables instead of id based

kodiakhq bot added a commit that referenced this issue Sep 21, 2021
Fixes #4224, partial fix for #3992

Description of changes:
- gather the Drude helpers global variables and free functions in a checkpointable class `DrudeHelpers`
- make the helper functions take particle handles instead of particle ids as arguments
@jngrad
Copy link
Member

jngrad commented Jan 24, 2022

After #4402, bond lists and particle-based observables are the last consumers of the particle ids in the python interface.

@RudolfWeeber
Copy link
Contributor

By now, to my understanding, only the particle observables are left. This can be addressed on the Python level.

#4954

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

No branches or pull requests

5 participants