feat: add methods to apply rewriter instructions to resources in routines by Brendan-Reid1991 · Pull Request #239 · PsiQ/bartiq

Brendan-Reid1991 · 2025-07-08T15:40:46Z

Description

Did some file restructuring.
- rewriters/expression.py now contains the base class
- rewriters/sympy_rewriter.py now contains the SympyExpressionRewriter and the factory method
- rewriters/resources.py now contains ResourceRewriter
Added tests for the routine-level logic.

I also want to settle on names for classes/files in this PR! So make your objections heard here!

Potential issues

The ResourceRewriter forwards attribute calls ton the rewriter attribute, and the routine attribute is not modified by instruction calls, only the rewriter is.
- Calling apply_to_entire_routine pushes all the instructions through every level of the routine, and returns a new CompiledRoutine object.
- This new object may have some substitutions that are not connected to any other attributes in its definition. The risk is that this object becomes detached from the workflow where it was generated, and essentially becomes a nonsense routine.
Usage deviation between ExpressionRewriters and ResourceRewriters
- Related to the above. Where certain methods on the ExpressionRewriter return a new ExpressionRewriter instance, the same is not true for ResourceRewriters
- Calling methods that return a new ExpressionRewriter on a ResourceRewriter updates the rewriter attribute, but the class instance remains the same.
- This creates a divergence in behaviour:

expr = max(0, a)
rewriter = sympy_rewriter(expr)
rewriter.assume("a>0") # a
rewriter.expression == max(0, a) # true

VS

routine = Routine(name='root', ..., resources_values = {"dummy": "max(0,a)}")
resource_rewriter = ResourceRewriter(routine, "dummy")
resource_rewriter.assume("a>0") # a
resource_rewriter.expression == a # true

That is, the ResourceRewriter instance is updated in-place, whereas the ExpressionRewriter is not.

Please verify that you have completed the following steps

I have self-reviewed my code.
I have included test cases validating introduced feature/fix.
I have updated documentation.

…ing and improved descriptions

…ork for all backends

* docs: Added the list of possible values for `ResourceType` and `PortDirection` on docstring * docs: Added missing `Raises` sections in existing docstrings * docs: Improved styling of function segments in docs for easy navigation * docs: Added clickable reference links to parts of documentations in doc strings * fix: Fixed the formatting and lint issues * fix(docs): Fixed docs reference issues on`_compile.py`, `_evaluate.py` & `_routine.py` * fix: Exposed `SymbolicBackend` through `bartiq.symbolics` * fix: Exposed `Port` at top level (`bartiq.Port`) * docs: Adding doc-string for `Port` class * fix(docs): Added missing Args, and references in modified docs * fix(docs): Added External references * fix(docs): Fixed issues of long `Return` descriptions * fix(docs): Reverting `symbolics` concept markdown content * fix(docs): Improved styling of overload functions displayed in docs & removed type hints for `postorder_transform` * fix(docs): Fixed a formatting issue in the module doc-string * fix(docs): Removed `SymbolicBackend` from exposing to top-level, and added submodule rendering and extra styling for documentation * fix(docs): Removed unnecessary doc-strings in each `postorder_transform` overload function * fix(docs): Extended `module` & `method` annotations in docs

…bout interactive tools

… + transitive resources

mstechly

Minor comments

mstechly · 2025-07-09T11:59:15Z

tests/analysis/rewriters/test_resources.py

+    return {"name": name, "resources": [{"name": "dummy", "type": "additive", "value": f"max(0, {name})"}]}
+
+
+b = routine("b")


[minor] IMO it would be clearer and more consistent with rest of tests if we would actually just put an explicit dictionary here?

yes, probably!

mstechly · 2025-07-09T12:05:02Z

src/bartiq/analysis/rewriters/resources.py

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Routine rewriters allow you to rewrite expressions across entire routines."""


[nitpick] I think it should be Resource rewriters now, no?

dexter2206 · 2025-07-11T12:27:47Z

src/bartiq/analysis/rewriters/resources.py

+        routine = deepcopy(self.routine)
+
+        def _traverse_routine(routine: CompiledRoutine) -> CompiledRoutine:
+            """Recursively traverse the routine, replacing resource values
+            starting from the lowest level of children."""
+            for _name, child_routine in routine.children.items():
+                routine.children[_name] = _traverse_routine(child_routine)
+            if self.resource in routine.resources and not isinstance(
+                routine.resource_values[self.resource], (int | float)
+            ):
+                return replace(
+                    routine,
+                    resources=routine.resources
+                    | {
+                        self.resource: replace(
+                            routine.resources[self.resource],
+                            value=_update_expression(
+                                self.rewriter_factory,
+                                cast(T, routine.resources[self.resource].value),
+                                self.rewriter.history(),
+                            ),
+                        )
+                    },
+                )
+
+            return routine


suggestion: There is no need for a deepcopy here. Observe that:

Updating of resources does not involve reading from children

Therefore, it doesn't matter if we have updated children or not when we are updating resources.

So, we can just store children and just update them when we do replace.

Even better, we already have similar transformations in other places like preprocessing, and we have a decorator called @postorder_transform that you can use to simplify this piece of code.

dexter2206 · 2025-07-11T12:34:36Z

tests/analysis/rewriters/test_resources.py

+root = {
+    "name": "root",
+    "children": [
+        {
+            "name": "a",
+            "children": [
+                {"name": "b", "resources": [{"name": "dummy", "type": "additive", "value": "max(0, b)"}]},
+                {"name": "c", "resources": [{"name": "dummy", "type": "additive", "value": "max(0, c)"}]},
+            ],
+        },
+        {
+            "name": "x",
+            "children": [
+                {"name": "y", "resources": [{"name": "dummy", "type": "additive", "value": "max(0, y)"}]},
+                {"name": "z", "resources": [{"name": "dummy", "type": "additive", "value": "max(0, z)"}]},
+            ],
+        },
+    ],
+}
+compiled = compile_routine(
+    Routine.from_qref(root, _SYMPY_BACKEND), compilation_flags=CompilationFlags.EXPAND_RESOURCES
+).routine


suggestion: Given some fields are mutable in both routine and compiled routine, I think that it would be much more safe to make those two variables into function-scoped fixtures.

Brendan-Reid1991 and others added 30 commits May 27, 2025 15:25

chore: added module level docstrings

e4fba55

chore: removed unused import

2bb75cd

chore: erroneous line introduced

41ab585

chore: added module level docstring

1b270ab

chore: improved typing in update_expression

6b3f738

chore: added module level docstring; removed return section of docstr…

26de84c

…ing and improved descriptions

chore: Type -> type

29bc2ca

Merge branch 'main' into symbolic_manipulator

e2e43f3

chore: made expression_rewriter.py public

25d4f8e

refactor: renamed variables -> free_symbols

08b64f2

refactor: improved docstrings

b483165

chore: correcting imports

ff2282a

refactor: renamed test_variables, and improved it such that it will w…

fa3e350

…ork for all backends

Merge branch 'main' into symbolic_manipulator

bd41a40

Merge branch 'main' into symbolic_manipulator

2c51d22

chore: added copyright message

6dfddb8

refactor: renamed free_symbols_in -> free_symbols

1a656b5

chore: updated docstrings

5310bea

chore: isort fix

d252149

feat: began development on assumptions

9f0c5a5

chore: Added a comment about allow_transitive_resources to tutorial #1

58eb2b7

chore: added allow_transitive_resources=False; added quotes to Note a…

442e6ab

…bout interactive tools

chore: updated tutorial 3

cb1f7b5

chore: Added sentences about transitive compilation of resources

d64f9aa

refactor: Created CompilationFlags enum

30c8c25

refactor: Replaced asserts with Exceptions; added fix for repetitions…

9fff558

… + transitive resources

chore: Updated docstrings

82df742

chore: updating tests for CompilationFlags

7b49072

chore: Modified tests to catch edge case errors (previously assertions)

59a8718

Brendan-Reid1991 added 2 commits July 8, 2025 16:14

added guard

e1a0ef7

removed Generic parent class from ResourceRewriter

32d51c0

cla-bot bot added the cla-signed label Jul 8, 2025

fixing mypy

f54ff8e

Brendan-Reid1991 changed the title ~~feat: add methods to apply rewriter instructions to resources in routines.~~ feat: add methods to apply rewriter instructions to resources in routines Jul 8, 2025

mstechly approved these changes Jul 9, 2025

View reviewed changes

Brendan-Reid1991 added 14 commits July 9, 2025 17:56

updating docstrings

5ab93c9

minor factor on substitutions

470690c

updating tests

a2a3d83

fixing attr error

35d078a

fixing tests

1728085

Merge branch 'substitutions' into methods_for_routine_rewriters

db58dfe

once more

1feefda

previous commit was made in err

ee4274c

removed testig code

3a917b5

Merge branch 'main' into substitutions

a1b1f09

Merge branch 'substitutions' into methods_for_routine_rewriters

0fb2746

michals comments

c7d47d5

updated assumption properties derivation; improved docstrings

09c9bee

updated tests

418f453

Brendan-Reid1991 requested a review from dexter2206 July 10, 2025 10:24

Merge branch 'substitutions' into methods_for_routine_rewriters

1b125cf

Base automatically changed from substitutions to main July 10, 2025 10:35

An error occurred while trying to automatically change base from substitutions to main July 10, 2025 10:35

Merge branch 'main' into methods_for_routine_rewriters

d9f9fa4

dexter2206 reviewed Jul 11, 2025

View reviewed changes

updated apply_to_whole_routine and tests

73744f7

Brendan-Reid1991 merged commit e54a94a into main Jul 14, 2025
9 checks passed

Brendan-Reid1991 deleted the methods_for_routine_rewriters branch July 14, 2025 10:32

release-please bot mentioned this pull request Jul 14, 2025

chore(main): release 0.14.0 #218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add methods to apply rewriter instructions to resources in routines#239

feat: add methods to apply rewriter instructions to resources in routines#239
Brendan-Reid1991 merged 285 commits intomainfrom
methods_for_routine_rewriters

Brendan-Reid1991 commented Jul 8, 2025 •

edited

Loading

Uh oh!

mstechly left a comment

Uh oh!

mstechly Jul 9, 2025

Uh oh!

Brendan-Reid1991 Jul 10, 2025

Uh oh!

mstechly Jul 9, 2025

Uh oh!

dexter2206 Jul 11, 2025

Uh oh!

Brendan-Reid1991 Jul 14, 2025

Uh oh!

dexter2206 Jul 11, 2025

Uh oh!

Brendan-Reid1991 Jul 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

		return {"name": name, "resources": [{"name": "dummy", "type": "additive", "value": f"max(0, {name})"}]}


		b = routine("b")

Conversation

Brendan-Reid1991 commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Potential issues

Please verify that you have completed the following steps

Uh oh!

mstechly left a comment

Choose a reason for hiding this comment

Uh oh!

mstechly Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

Brendan-Reid1991 Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

mstechly Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

dexter2206 Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Brendan-Reid1991 Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

dexter2206 Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Brendan-Reid1991 Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

Brendan-Reid1991 commented Jul 8, 2025 •

edited

Loading