More review comments in expression mixins

11 files changed, 46 insertions, 12 deletions

diff --git a/polymatrix/expression/mixins/additionexprmixin.py b/polymatrix/expression/mixins/additionexprmixin.py
index bc83ff3..200c328 100644
--- a/polymatrix/expression/mixins/additionexprmixin.py
+++ b/polymatrix/expression/mixins/additionexprmixin.py
@@ -44,6 +44,8 @@ class AdditionExprMixin(ExpressionBaseMixin):
 
         poly_matrix_data = {}
 
+        # NP: this code is very old I presume, so it iterates over zero entries
+        # FIXME: iterate only over non-zero entries
         for row in range(left.shape[0]):
             for col in range(left.shape[1]):
                 poly_data = {}
diff --git a/polymatrix/expression/mixins/blockdiagexprmixin.py b/polymatrix/expression/mixins/blockdiagexprmixin.py
index 4688139..18af1f3 100644
--- a/polymatrix/expression/mixins/blockdiagexprmixin.py
+++ b/polymatrix/expression/mixins/blockdiagexprmixin.py
@@ -23,18 +23,20 @@ class BlockDiagExprMixin(ExpressionBaseMixin):
     def apply(
         self,
         state: ExpressionState,
-    ) -> tuple[ExpressionState, PolyMatrix]:
+    ) -> tuple[ExpressionState, PolyMatrix]: # FIXME: incorrect return type
         all_underlying = []
         for expr in self.underlying:
             state, polymat = expr.apply(state=state)
             all_underlying.append(polymat)
 
+        # NP: this is a very weird place to put a class
         @dataclassabc.dataclassabc(frozen=True)
         class BlockDiagPolyMatrix(PolyMatrixMixin):
             all_underlying: tuple[PolyMatrixMixin]
             underlying_row_col_range: tuple[tuple[int, int], ...]
             shape: tuple[int, int]
 
+            # FIXME: typing problems
             def get_poly(self, row: int, col: int) -> dict[tuple[int, ...], float]:
                 for polymatrix, ((row_start, col_start), (row_end, col_end)) in zip(
                     self.all_underlying, self.underlying_row_col_range
@@ -49,6 +51,7 @@ class BlockDiagExprMixin(ExpressionBaseMixin):
                         else:
                             return None
 
+                # NP: Do not raise generic expression, specialize error
                 raise Exception(f"row {row} is out of bounds")
 
         underlying_row_col_range = tuple(
diff --git a/polymatrix/expression/mixins/cacheexprmixin.py b/polymatrix/expression/mixins/cacheexprmixin.py
index 94cd7dc..7947a74 100644
--- a/polymatrix/expression/mixins/cacheexprmixin.py
+++ b/polymatrix/expression/mixins/cacheexprmixin.py
@@ -12,14 +12,14 @@ class CacheExprMixin(ExpressionBaseMixin):
     """Caches the polynomial matrix using the state"""
 
     @property
-    @abc.abstractclassmethod
+    @abc.abstractclassmethod # FIXME: not a classmethod
     def underlying(self) -> ExpressionBaseMixin: ...
 
     # overwrites the abstract method of `ExpressionBaseMixin`
     def apply(
         self,
         state: ExpressionStateMixin,
-    ) -> tuple[ExpressionStateMixin, PolyMatrixMixin]:
+    ) -> tuple[ExpressionStateMixin, PolyMatrixMixin]: # FIXME: return type
         if self in state.cache:
             return state, state.cache[self]
 
diff --git a/polymatrix/expression/mixins/combinationsexprmixin.py b/polymatrix/expression/mixins/combinationsexprmixin.py
index 8a99455..56a1d64 100644
--- a/polymatrix/expression/mixins/combinationsexprmixin.py
+++ b/polymatrix/expression/mixins/combinationsexprmixin.py
@@ -14,6 +14,8 @@ class CombinationsExprMixin(ExpressionBaseMixin):
 
     [[x]]  ->  [[1], [x], [x**2], [x**3]]
     """
+    # NP: example is not great, should be with x_1 and x_2 to show actual
+    # NP: effect of combinations (or am I understanding this wrong?)
 
     @property
     @abc.abstractmethod
diff --git a/polymatrix/expression/mixins/degreeexprmixin.py b/polymatrix/expression/mixins/degreeexprmixin.py
index e847102..5518d0a 100644
--- a/polymatrix/expression/mixins/degreeexprmixin.py
+++ b/polymatrix/expression/mixins/degreeexprmixin.py
@@ -8,6 +8,9 @@ from polymatrix.expressionstate.abc import ExpressionState
 from polymatrix.utils.getstacklines import FrameSummary
 
 
+# NP: IIRC this gets the highest degree.
+# NP: If I am correct consider renaming to MaxDegree
+# NP: (also what is the use case?)
 class DegreeExprMixin(ExpressionBaseMixin):
     @property
     @abc.abstractmethod
diff --git a/polymatrix/expression/mixins/determinantexprmixin.py b/polymatrix/expression/mixins/determinantexprmixin.py
index 5694ddd..2c034d1 100644
--- a/polymatrix/expression/mixins/determinantexprmixin.py
+++ b/polymatrix/expression/mixins/determinantexprmixin.py
@@ -14,6 +14,7 @@ class DeterminantExprMixin(ExpressionBaseMixin):
     @abc.abstractmethod
     def underlying(self) -> ExpressionBaseMixin: ...
 
+    # NP: delete this code?
     # # overwrites the abstract method of `ExpressionBaseMixin`
     # @property
     # def shape(self) -> tuple[int, int]:
@@ -39,13 +40,20 @@ class DeterminantExprMixin(ExpressionBaseMixin):
         index_start = state.n_param
         rel_index = 0
 
+        # NP: consider providing a reference eg algorithm name if it has one
+        # NP: or at least a minimal overview / explaination.
+
         for row in range(underlying.shape[0]):
+            # FIXME: type annotations of dictionaries inside method
             polynomial = collections.defaultdict(float)
 
             # f in f-v^T@x-r^2
             # terms = underlying.get_poly(row, row)
             try:
                 underlying_poly = underlying.get_poly(row, row)
+
+            # NP: this is polymatrix leaking the abstraction, you should not
+            # NP: need to care about these problems here. => Fix inside PolyMatrix
             except KeyError:
                 pass
             else:
@@ -99,6 +107,12 @@ class DeterminantExprMixin(ExpressionBaseMixin):
             rel_index += row
             inequality_data[row, 0] = dict(polynomial)
 
+        # NP: Type checker does not like the fact that state is of type
+        # NP: ExpressionState, while register returns ExpressionStateMixinis this
+        # NP: was a common problem I had while coding mdpoly, that eventually led
+        # NP: me to abandoning mixins because they are a pain to get right with typing 
+        # NP: such that the type checker (mypy) is happy. This problem happens all over the
+        # NP: place, I don't have a quick solution.
         state = state.register(rel_index)
 
         poly_matrix = init_poly_matrix(
diff --git a/polymatrix/expression/mixins/diagexprmixin.py b/polymatrix/expression/mixins/diagexprmixin.py
index 826cb94..1feec9d 100644
--- a/polymatrix/expression/mixins/diagexprmixin.py
+++ b/polymatrix/expression/mixins/diagexprmixin.py
@@ -20,6 +20,7 @@ class DiagExprMixin(ExpressionBaseMixin):
     def underlying(self) -> ExpressionBaseMixin: ...
 
     # overwrites the abstract method of `ExpressionBaseMixin`
+    # FIXME: typing
     def apply(
         self,
         state: ExpressionStateMixin,
@@ -37,6 +38,8 @@ class DiagExprMixin(ExpressionBaseMixin):
                     if row == col:
                         return self.underlying.get_poly(row, 0)
                     else:
+                        # FIXME: should return none according to base class
+                        # NP: Though returning zero makes more sense
                         return {tuple(): 0.0}
 
             return state, DiagPolyMatrix(
@@ -45,8 +48,10 @@ class DiagExprMixin(ExpressionBaseMixin):
             )
 
         else:
+            # NP: replace assertions with meaningful exception
             assert underlying.shape[0] == underlying.shape[1], f"{underlying.shape=}"
 
+            # NP: why is this called Trace?
             @dataclassabc.dataclassabc(frozen=True)
             class TracePolyMatrix(PolyMatrixMixin):
                 underlying: PolyMatrixMixin
diff --git a/polymatrix/expression/mixins/divergenceexprmixin.py b/polymatrix/expression/mixins/divergenceexprmixin.py
index b4deb25..3477ee1 100644
--- a/polymatrix/expression/mixins/divergenceexprmixin.py
+++ b/polymatrix/expression/mixins/divergenceexprmixin.py
@@ -29,6 +29,7 @@ class DivergenceExprMixin(ExpressionBaseMixin):
         state, underlying = self.underlying.apply(state=state)
         state, variables = get_variable_indices_from_variable(state, self.variables)
 
+        # NP: replace with exception with error explaination
         assert underlying.shape[1] == 1, f"{underlying.shape=}"
         assert (
             len(variables) == underlying.shape[0]
@@ -37,8 +38,9 @@ class DivergenceExprMixin(ExpressionBaseMixin):
         polynomial_data = collections.defaultdict(float)
 
         for row, variable in enumerate(variables):
+            # NP: did you know that python has the walrus operator? `:=`
+            # NP: if poly := underlying.get_poly(row, 0):
             polynomial = underlying.get_poly(row, 0)
-
             if polynomial is None:
                 continue
 
diff --git a/polymatrix/expression/utils/broadcastpolymatrix.py b/polymatrix/expression/utils/broadcastpolymatrix.py
index c24e31e..7bfde13 100644
--- a/polymatrix/expression/utils/broadcastpolymatrix.py
+++ b/polymatrix/expression/utils/broadcastpolymatrix.py
@@ -4,11 +4,10 @@ from polymatrix.utils.tooperatorexception import to_operator_exception
 from polymatrix.polymatrix.abc import PolyMatrix
 
 
-# NP: "broadcast" does not mean scalar to me cf. BroadcastPolyMatrix
-# NP: broadcast means only to change into something else cf. numpy.broadcast_to
-# NP: consider renaming to broadcast_to_scalar or just get rid of broadcast
-# NP: altogether and call it to_scalar
-# FIXME: typing problems
+# NP: "broadcast" is not well defined cf. BroadcastPolyMatrix
+# NP: Is this supposed to behave exactly like like in numpy?
+# NP: cf. https://numpy.org/doc/stable/user/basics.broadcasting.html
+# FIXME: typing problems, incorrect return type
 def broadcast_poly_matrix(
     left: PolyMatrix,
     right: PolyMatrix,
diff --git a/polymatrix/expression/utils/getvariableindices.py b/polymatrix/expression/utils/getvariableindices.py
index 743f6b9..aebdeb0 100644
--- a/polymatrix/expression/utils/getvariableindices.py
+++ b/polymatrix/expression/utils/getvariableindices.py
@@ -5,7 +5,9 @@ from polymatrix.expressionstate.abc import ExpressionState
 from polymatrix.expression.mixins.expressionbasemixin import ExpressionBaseMixin
 
 
-# FIXME: typing
+# NP: very confusing name without explaination / context
+# NP: why is this not a feature of expressionstate?
+# FIXME: typing, especially return type
 def get_variable_indices_from_variable(
     state: ExpressionState,
     variable: ExpressionBaseMixin | int | typing.Any,
diff --git a/polymatrix/utils/deprecation.py b/polymatrix/utils/deprecation.py
index c9dea71..3e4302a 100644
--- a/polymatrix/utils/deprecation.py
+++ b/polymatrix/utils/deprecation.py
@@ -1,5 +1,7 @@
-""" Provides tools to mark parts of code a deprecated, for a smoother
-transition to new code. """
+"""
+Provides tools to mark parts of code a deprecated, for a smoother transition to
+new code.
+"""
 from __future__ import annotations
 from typing import Callable, Any, overload
 from warnings import warn