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

Merge conservation_rules and rule_priorities attributes #279

Open
redeboer opened this issue Aug 9, 2024 · 5 comments
Open

Merge conservation_rules and rule_priorities attributes #279

redeboer opened this issue Aug 9, 2024 · 5 comments
Assignees
Labels
⚠️ Interface Breaking changes to the API ❔ Question Discuss this matter in the team
Milestone

Comments

@redeboer
Copy link
Member

redeboer commented Aug 9, 2024

While investigating #272, I started to wonder why EdgeSettings and NodeSettings require conservation_rules as well as rule_priorities. Can't these two just be combined into one attribute of type dict[Rule, int]?

@define
class EdgeSettings:
"""Solver settings for a specific edge of a graph."""
conservation_rules: set[GraphElementRule] = field(factory=set)
rule_priorities: dict[GraphElementRule, int] = field(factory=dict)
qn_domains: dict[Any, list] = field(factory=dict)

@redeboer redeboer added ❔ Question Discuss this matter in the team ⚠️ Interface Breaking changes to the API labels Aug 9, 2024
@redeboer redeboer added this to the 0.11.0 milestone Aug 9, 2024
@redeboer redeboer removed their assignment Aug 14, 2024
@grayson-helmholz
Copy link
Contributor

Aside from tests, Edge/NodeSettings is mainly used in settings.create_interaction_settings(...) but which is then heavily employed in the StateTransitionManager. Should this be in its own branch?

@redeboer
Copy link
Member Author

Should this be in its own branch?

Yes as far as possible it is better to keep refactorings isolated and small

@redeboer
Copy link
Member Author

redeboer commented Aug 27, 2024

Some idea after discussions: we could write something like:

conservation_rules:  dict[GraphElementRule, RulePriorities]

where RulePriorities is Annotated

@grayson-helmholz
Copy link
Contributor

Rewriting get_rules_by_priority() with the new definitions of Edge|NodeSettings
causes an infinite-loop in tests/channels/test_y_to_d0_d0bar_pi0_pi0.py

@redeboer
Copy link
Member Author

redeboer commented Sep 3, 2024

Funny, seems that the rule priorities never did anything from the start 😂

(x, graph_element_settings.rule_priorities[type(x)]) # type: ignore[index]
if type(x) in graph_element_settings.rule_priorities
else (x, 1)

The keys are x, not type(x), so it always falls back to 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ Interface Breaking changes to the API ❔ Question Discuss this matter in the team
Projects
None yet
Development

No branches or pull requests

2 participants