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

feat: change state to userdict #44

Merged
merged 4 commits into from
Sep 3, 2023

Conversation

younesStrittmatter
Copy link
Contributor

Description

Change state into a user dict

Type of change

  • feat: A new feature

  • BREAKING_CHANGE: Not sure if it breaks existing code, but tests/notebooks in the core worked fine

Remarks (Optional)

  • The old state class is still implemented and State, StandardState are now aliases of the "Dict"-Versions. If we find Problems, we can easily go back to the Dataclass solution
  • The advantage of this is, that the fields of the state can now be changed dynamically. For example, one can add fields easily and alter their "delta"-behaviour between cycles

Copy link
Contributor

@musslick musslick left a comment

Choose a reason for hiding this comment

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

This looks great! The only suggestion I have is to add a minimal jupyter notebook under docs/cycle that outlines the new functionality, i.e., how the state can be modified (adding fields, changing delta functions).

Copy link
Contributor

@musslick musslick left a comment

Choose a reason for hiding this comment

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

Looks great!

@younesStrittmatter younesStrittmatter added this pull request to the merge queue Sep 3, 2023
Merged via the queue into main with commit 743cb4a Sep 3, 2023
13 checks passed
@younesStrittmatter younesStrittmatter deleted the feat-user-dict-instead-of-dataclass branch September 3, 2023 11:40
Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

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

I think this needs a bunch of automated tests before it gets merged — it's such a central part of the core, that we really need confidence it's working!

@@ -45,8 +45,95 @@ def __add__(self: C, other: Union[Delta, Mapping]) -> C:
S = TypeVar("S", bound=DeltaAddable)


class StateDict(UserDict):
Copy link
Member

Choose a reason for hiding this comment

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

I think this class would benefit from a bunch of doctests which demo and check it's full functionality! You could start with the doctests in the StandardState as an example, and then add the new functionality too.

hollandjg added a commit that referenced this pull request Nov 29, 2023
…onal-approach

docs: update docs with functional workflow approach examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants