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

Explicit operation order determined by user definition #379

Merged
merged 7 commits into from
May 11, 2024

Conversation

skim0119
Copy link
Collaborator

@skim0119 skim0119 commented May 9, 2024

Explicit operation order

  • User-API would remain the same

Description

Previous implementation has ambiguity in determining the order of operation for different boundary conditions: forcing, contact, friction, connection, etc. For example, contact and friction must be calculated after all other boundary conditions are in place, and strictly contact must be computed before friction since friction depends on contact. In various place in the code, we tried to rearange the order without explicitly letting user know:

_call_contacts_index = []
for idx, feature in enumerate(self._feature_group_synchronize):
if feature.__name__ == "_call_contacts":
_call_contacts_index.append(idx)
# Move to the _call_contacts to the end of the _feature_group_synchronize list.
for index in _call_contacts_index:
self._feature_group_synchronize.append(
self._feature_group_synchronize.pop(index)
)
,
# Find if there are any friction plane forcing, if add them to the end of the list,
# since friction planes uses external forces.
friction_plane_index = []
for idx, ext_force_torque in enumerate(self._ext_forces_torques):
if isinstance(ext_force_torque[1], AnisotropicFrictionalPlane):
friction_plane_index.append(idx)
# Move to the friction forces to the end of the external force and torques list.
for index in friction_plane_index:
self._ext_forces_torques.append(self._ext_forces_torques.pop(index))
.
This cause ambiguity on user, especially when user wants to define their own boundary condition.

Solution

As long as we let users to write their own boundary conditions, we cannot anticipate every possible cases of operation dependencies. We are shifting to the policy that user must be aware of the boundary operation order.

This PR removes any "back-end" sorting the operation order. Operation order is now strictly defined by the order of user inserting any boundary condition with .using. This applies to forcing, contact, and connection as of now.

Some remaining TODOs:

  • Add warning messages on packaged contact and friction modules
  • Refactor synchronize operations: "first-in" boundary condition computes first
  • Test
    • Fix existing tests
    • Implement new test for testing operation order
  • Documentation (f0fc544)

@skim0119 skim0119 changed the base branch from update/mypy to update-0.3.3 May 9, 2024 08:04
@skim0119 skim0119 force-pushed the feature/order-operation branch from f6d4b97 to ca83da1 Compare May 9, 2024 08:07
@skim0119 skim0119 self-assigned this May 9, 2024
@skim0119 skim0119 added enhancement New feature or request prio:high Priority level: high labels May 9, 2024
@skim0119 skim0119 force-pushed the feature/order-operation branch 3 times, most recently from e5fa496 to 00d6caa Compare May 9, 2024 08:16
@skim0119 skim0119 force-pushed the feature/order-operation branch from 00d6caa to 44d836a Compare May 9, 2024 08:18
@skim0119 skim0119 force-pushed the feature/order-operation branch from 8151230 to 239fb0c Compare May 9, 2024 17:30
@skim0119 skim0119 changed the title Fix: Make operation order fully determined by user definition Explicit operation order determined at user definition May 9, 2024
@skim0119 skim0119 changed the title Explicit operation order determined at user definition Explicit operation order determined by user definition May 9, 2024
@skim0119 skim0119 marked this pull request as ready for review May 9, 2024 17:52
@armantekinalp armantekinalp requested review from Ali-7800 and sy-cui May 9, 2024 19:54
Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

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

I have some questions. But overall looks good.

elastica/modules/feature_group.py Outdated Show resolved Hide resolved
elastica/modules/connections.py Show resolved Hide resolved
elastica/modules/contact.py Show resolved Hide resolved
@skim0119 skim0119 force-pushed the feature/order-operation branch from c4ad237 to 5a68fc6 Compare May 10, 2024 06:05
@skim0119 skim0119 requested a review from armantekinalp May 10, 2024 06:08
Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

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

LGTM

@skim0119 skim0119 force-pushed the feature/order-operation branch from e71e547 to 2a6ddfa Compare May 11, 2024 02:26
@skim0119 skim0119 merged commit 9a92a49 into update-0.3.3 May 11, 2024
11 checks passed
@skim0119 skim0119 deleted the feature/order-operation branch May 11, 2024 02:32
@skim0119 skim0119 mentioned this pull request May 11, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request prio:high Priority level: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants