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

Reduce direct use of numerical part id (doc, samples) #4058

Merged
merged 9 commits into from
Dec 20, 2020

Conversation

pkreissl
Copy link
Contributor

@pkreissl pkreissl commented Dec 17, 2020

Part of #4029

Description of changes:

  • make vs_auto_relate_to() take ParticleHandle, not just particle id
  • remove particle id from doc and samples wherever possible
  • improve checking of arguments for VS and bonds (e.g. prevent adding the same bond twice by calling add_bond() with a particle id and calling it again with the corresponding particle handle)

There are still some places, especially in the espressomd.observables, where for now particle ids cannot be omitted, such as ParticlePositions.

@pkreissl pkreissl force-pushed the marginalize_part_id branch from 81a8514 to 1c20ebd Compare December 17, 2020 17:49
KaiSzuttor
KaiSzuttor previously approved these changes Dec 17, 2020
Copy link
Member

@KaiSzuttor KaiSzuttor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 really good stuff


"""
check_type_or_throw_except(
_relto, 1, int, "Argument of vs_auto_relate_to has to be of type int.")
# If _relto is of type ParticleHandle,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none of your business but this variable name is not too descriptive and the underscore is not pep8... just if your over-motivated :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, the current variable names in this part of the interface are not as I would like them to be. However, this is true for all member variables of the ParticleHandle class. I think the idea here is to mirror the core variable but adding a prefixed underscore to not have the variables being exactly the same. I think @RudolfWeeber commented in some PR (that I can't find just know), that he would like to have distinct names for the variables in core and interface respectively. If I am wrong here, sure I can quickly go over the class and "pepify" it a bit.

RudolfWeeber
RudolfWeeber previously approved these changes Dec 19, 2020
@jngrad jngrad added this to the Espresso 4.2 milestone Dec 19, 2020
@jngrad jngrad added automerge Merge with kodiak BugFix labels Dec 19, 2020
@jngrad jngrad added automerge Merge with kodiak and removed automerge Merge with kodiak labels Dec 20, 2020
@jngrad jngrad added the automerge Merge with kodiak label Dec 20, 2020
@kodiakhq kodiakhq bot merged commit 7765ce0 into espressomd:python Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants