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

Rewrite non-bonded interactions as script interface objects #4558

Merged
merged 24 commits into from
Sep 20, 2022

Conversation

jhossbach
Copy link
Contributor

@jhossbach jhossbach commented Aug 24, 2022

Fixes #4555 and fixes #4569

Description of changes:

  • rewrite non-bonded interactions as script interface objects
    • convert Cython file interaction.pyx to Python file interaction.py
    • 1600 lines of Cython code were removed
  • bugfix: the set_params() method now raises an error if required arguments are not provided
  • new feature: possibility to deactivate/reset non-bonded interactions
  • reduce code duplication in the script interface of bonded interactions

@jngrad jngrad force-pushed the nonbonded_refactor_new branch 2 times, most recently from c4b2640 to 29ef411 Compare August 24, 2022 17:14
@jngrad jngrad force-pushed the nonbonded_refactor_new branch from 29ef411 to a9f5a24 Compare August 26, 2022 12:33
@jngrad jngrad changed the title WIP: Rewrite non-bonded interactions as script interface objects Rewrite non-bonded interactions as script interface objects Aug 29, 2022
@jngrad jngrad force-pushed the nonbonded_refactor_new branch from e4abb5a to 302ffc4 Compare September 5, 2022 21:03
@jngrad jngrad force-pushed the nonbonded_refactor_new branch from 302ffc4 to 53368cc Compare September 5, 2022 21:40
@jngrad jngrad force-pushed the nonbonded_refactor_new branch 2 times, most recently from 3a3c2ff to 83a4c98 Compare September 9, 2022 17:24
@jngrad jngrad force-pushed the nonbonded_refactor_new branch from 83a4c98 to 8078029 Compare September 12, 2022 11:23
@jngrad jngrad marked this pull request as ready for review September 12, 2022 13:38
@jngrad jngrad added this to the Espresso 4.3.0 milestone Sep 12, 2022
@jngrad jngrad requested a review from reinaual September 12, 2022 13:39
Comment on lines 89 to 90
std::swap(::old_nonbonded_ia_params, new_params_);
std::swap(::nonbonded_ia_params, new_params);
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a swap here? Probably a move-assignment would be more expressive

Copy link
Member

Choose a reason for hiding this comment

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

I just looked it up and apparently, a swap was the only way to do a cheap move for std::vector back in C++03. I'll change it to a std::move.

@@ -205,6 +205,85 @@ cdef class NonBondedInteraction:
raise Exception(
"Subclasses of NonBondedInteraction must define the required_keys() method.")


class NewNonBondedInteraction(ScriptInterfaceHelper):
Copy link
Contributor

@reinaual reinaual Sep 19, 2022

Choose a reason for hiding this comment

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

Have we ever tested the python abstract base class to work together with the ScriptInterface? Probably a lot of the methods would be perfect candidates for @abc.abstractmethod

Copy link
Member

Choose a reason for hiding this comment

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

Well, it wasn't used before because everything was in Cython, and abstract classes don't work out-of-the-box in Cython:

Error compiling Cython file:
------------------------------------------------------------
...

import abc
cdef class HydrodynamicInteraction(FluidActor, metaclass=abc.ABCMeta):
                                                                    ^
------------------------------------------------------------
/work/jgrad/es/src/python/espressomd/lb.pyx:172:69: C classes cannot take keyword bases.

or

Error compiling Cython file:
------------------------------------------------------------
...

import abc
cdef class FluidActor(abc.ABC):
                        ^
------------------------------------------------------------
/work/jgrad/es/src/python/espressomd/lb.pyx:34:25: First base of 'FluidActor' is not an extension type

The usual workaround was to declare a python abstract class and use it to register the cdef class to make it abstract, which is less elegant and error-prone. Now that the module is in Python, we can use ABC metaclasses.

Co-authored-by: Alexander Reinauer <[email protected]>
reinaual
reinaual previously approved these changes Sep 20, 2022
@jngrad jngrad added the automerge Merge with kodiak label Sep 20, 2022
@kodiakhq kodiakhq bot merged commit 74b9092 into espressomd:python Sep 20, 2022
kodiakhq bot added a commit that referenced this pull request Oct 14, 2022
Description of changes:
- fix a regression introduced in 4.3-dev by 283e0c8 in #4558 that crashes the visualizer when drawing bonds between particles
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.

Non-bonded interactions set_params() is broken Rewrite non-bonded interactions as script interface objects
3 participants