Skip to content

Fixed a Possible Bug in the Extended State Fusion #2009

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

philip-paul-mueller
Copy link
Collaborator

The variable connections_to_make was a class variable and not marked as a dace property and hence shared between all instances. It is now an instance variable and thus private.
Also the variable was never cleared, so technically, the connections should have accumulated inside it.

Furthermore, the transformation assumed that can_be_applied() was called immediately before apply(), because the can_be_applied() function populates the connections_to_make. However, this behaviour is not guaranteed and now apply() explicitly calls can_be_applied() to make sure that connections_to_make is properly populated.

The variable `connections_to_make` was a class variable and not marked as a dace property and hence shared between all instances.
It is now an instance variable and thus private.
Also the variable was never cleared, so technically, the connections should have accumulated inside it.

Furthermore, the transformation assumed that `can_be_applied()` was called immediately before `apply()`, because the `can_be_applied()` function populates the `connections_to_make`.
However, this behaviour is not guaranteed and now `apply()` explicitly calls `can_be_applied()` to make sure that `connections_to_make` is properly populated.
Copy link
Contributor

@acalotoiu acalotoiu left a comment

Choose a reason for hiding this comment

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

LGTM, can you please add a new test to this PR to showcase the bug and how this fixed it? Thank you!

@philip-paul-mueller
Copy link
Collaborator Author

I am not fully sure how a unit test for that thing would look like, as it essentially would be a test that there is no (observable) state, between two invocation of the same object.

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 this pull request may close these issues.

2 participants