Repository: matthewwardrop/formulaic

Issue:
`_ordering="none"` is ignored when a dependent variable is included
Thanks for the very useful library!

I'm having some issues with retaining the order of the terms as they are in the formula. Using the example from the README, but with a `c  ~` added, yield the following result:

```python
In [2]: {
   ...:     "degree": Formula("c ~ z + z:a + z:b:a + g").rhs,
   ...:     "none": Formula("c ~ z + z:a + z:b:a + g", _ordering="none").rhs,
   ...:     "sort": Formula("c ~ z + z:a + z:b:a + g", _ordering="sort").rhs,
   ...: }
Out[2]:
{'degree': 1 + z + g + z:a + z:b:a,
 'none': 1 + z + g + z:a + z:b:a,
 'sort': 1 + g + z + a:z + a:b:z}
```

Notice the `'none'` example is identical to the `'degree'` (default) option. Is there a reason for this?

Discussion:
@matthewwardrop: Hmmm... good observation, especially since the behaviour in the docs indicates otherwise. I'll take a look at this shortly... I suspect that `_ordering` is just not being propagated correctly.@matthewwardrop: This is now fixed in main, and will be released in the next version. Thanks for reporting!

Solution patch:
diff --git a/formulaic/formula.py b/formulaic/formula.py
index 7557c39c..eb422dc6 100644
--- a/formulaic/formula.py
+++ b/formulaic/formula.py
@@ -144,6 +144,8 @@ def _prepare_item(self, key: str, item: FormulaSpec) -> Union[List[Term], Formul
             item: The specification to convert.
         """
 
+        formula_or_terms: Union[List[Term], Structured[List[Term]]]
+
         if isinstance(item, str):
             item = cast(
                 FormulaSpec,
@@ -153,9 +155,14 @@ def _prepare_item(self, key: str, item: FormulaSpec) -> Union[List[Term], Formul
             )
 
         if isinstance(item, Structured):
-            formula_or_terms = Formula(
-                _parser=self._nested_parser, **item._structure
-            )._simplify()
+            return cast(
+                Union[List[Term], Formula],
+                Formula(
+                    _parser=self._nested_parser,
+                    _ordering=self._ordering,
+                    **item._structure,
+                )._simplify(),
+            )
         elif isinstance(item, (list, set, OrderedSet)):
             formula_or_terms = [
                 term
@@ -266,14 +273,14 @@ def __getattr__(self, attr: str) -> Any:
         # Keep substructures wrapped to retain access to helper functions.
         subformula = super().__getattr__(attr)
         if attr != "root":
-            return Formula.from_spec(subformula)
+            return Formula.from_spec(subformula, ordering="none")  # already ordered
         return subformula
 
     def __getitem__(self, key: Any) -> Any:
         # Keep substructures wrapped to retain access to helper functions.
         subformula = super().__getitem__(key)
         if key != "root":
-            return Formula.from_spec(subformula)
+            return Formula.from_spec(subformula, ordering="none")  # already ordered
         return subformula
 
     def __repr__(self, to_str: Callable[..., str] = repr) -> str:
diff --git a/formulaic/parser/types/structured.py b/formulaic/parser/types/structured.py
index da290e3f..2d1e0f01 100644
--- a/formulaic/parser/types/structured.py
+++ b/formulaic/parser/types/structured.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import copy
 import itertools
 from collections import defaultdict
 from typing import (
@@ -97,7 +98,7 @@ def __init__(
                 f"The invalid keys are: {set(key for key in structure if key.startswith('_'))}."
             )
         if root is not _MISSING:
-            structure["root"] = self.__prepare_item("root", root)
+            structure["root"] = root
         self._metadata = _metadata
 
         self._structure = {
@@ -106,6 +107,9 @@ def __init__(
 
     def __prepare_item(self, key: str, item: Any) -> Any:
         if isinstance(item, Structured):
+            if type(item) is self.__class__:
+                # Will already have been prepared
+                return item
             return item._map(
                 lambda x: self._prepare_item(key, x), as_type=self.__class__
             )
@@ -258,28 +262,39 @@ def _simplify(
         if not isinstance(structured, Structured):
             return structured
 
-        structure = structured._structure
+        structure = structured._structure.copy()
+        structure_modified: bool = False
 
         if recurse:
 
-            def simplify_obj(obj: Any) -> Any:
+            def simplify_obj(obj: Any) -> Tuple[Any, bool]:
+                """
+                Return the simplified object, and a flag indicating whether the
+                object was modified.
+                """
                 if isinstance(obj, Structured):
-                    return obj._simplify(recurse=True)
+                    simplified = obj._simplify(recurse=True)
+                    return simplified, simplified is not obj
                 if isinstance(obj, tuple):
-                    return tuple(simplify_obj(o) for o in obj)
-                return obj
-
-            structure = {
-                key: simplify_obj(value) for key, value in structured._structure.items()
-            }
-
-        if inplace:
-            self._structure = structure
-            return self
-        return self.__class__(
-            _metadata=self._metadata,
-            **structure,
-        )
+                    simplified = tuple(simplify_obj(o) for o in obj)
+                    return tuple(s[0] for s in simplified), any(
+                        s[1] for s in simplified
+                    )
+                return obj, False
+
+            for key, value in tuple(structure.items()):
+                value, value_modified = simplify_obj(value)
+                if value_modified:
+                    structure[key] = value
+                    structure_modified = True
+
+        if not inplace and not structure_modified:
+            # Avoid any further work if simplification has not occurred
+            return structured
+        if not inplace:
+            self = copy.copy(self)
+        self._structure = structure
+        return self
 
     def _update(self, root: Any = _MISSING, **structure: Any) -> Structured[ItemType]:
         """


Test patch:
diff --git a/tests/parser/types/test_structured.py b/tests/parser/types/test_structured.py
index 30d54c3e..f7724c9c 100644
--- a/tests/parser/types/test_structured.py
+++ b/tests/parser/types/test_structured.py
@@ -1,6 +1,5 @@
 import pickle
 import re
-from ast import Str
 from io import BytesIO
 
 import pytest
diff --git a/tests/test_formula.py b/tests/test_formula.py
index ffd79803..15210cab 100644
--- a/tests/test_formula.py
+++ b/tests/test_formula.py
@@ -68,34 +68,65 @@ def test_terms(self, formula_expr):
         ]
 
     def test_ordering(self):
-        assert [str(t) for t in Formula("a+e:d+b:c+f")] == [
+        # assert [str(t) for t in Formula("a+e:d+b:c+f")] == [
+        #     "1",
+        #     "a",
+        #     "f",
+        #     "e:d",
+        #     "b:c",
+        # ]
+        # assert [str(t) for t in Formula("a+e:d+b:c+f", _ordering="degree")] == [
+        #     "1",
+        #     "a",
+        #     "f",
+        #     "e:d",
+        #     "b:c",
+        # ]
+        # assert [str(t) for t in Formula("a+e:d+b:c+f", _ordering="none")] == [
+        #     "1",
+        #     "a",
+        #     "e:d",
+        #     "b:c",
+        #     "f",
+        # ]
+        # assert [str(t) for t in Formula("a+e:d+b:c+f", _ordering="sort")] == [
+        #     "1",
+        #     "a",
+        #     "f",
+        #     "b:c",
+        #     "d:e",
+        # ]
+
+        # Test nested ordering
+
+        # assert [str(t) for t in Formula("y~a+e:d+b:c+f").rhs] == [
+        #     "1",
+        #     "a",
+        #     "f",
+        #     "e:d",
+        #     "b:c",
+        # ]
+        # assert [str(t) for t in Formula("y~a+e:d+b:c+f", _ordering="degree").rhs] == [
+        #     "1",
+        #     "a",
+        #     "f",
+        #     "e:d",
+        #     "b:c",
+        # ]
+        assert [str(t) for t in Formula("y~a+e:d+b:c+f", _ordering="none").rhs] == [
             "1",
             "a",
-            "f",
             "e:d",
             "b:c",
-        ]
-        assert [str(t) for t in Formula("a+e:d+b:c+f", _ordering="degree")] == [
-            "1",
-            "a",
             "f",
-            "e:d",
-            "b:c",
-        ]
-        assert [str(t) for t in Formula("a+e:d+b:c+f", _ordering="none")] == [
-            "1",
-            "a",
-            "e:d",
-            "b:c",
-            "f",
-        ]
-        assert [str(t) for t in Formula("a+e:d+b:c+f", _ordering="sort")] == [
-            "1",
-            "a",
-            "f",
-            "b:c",
-            "d:e",
         ]
+        # assert [str(t) for t in Formula("y~a+e:d+b:c+f", _ordering="sort").rhs] == [
+        #     "1",
+        #     "a",
+        #     "f",
+        #     "b:c",
+        #     "d:e",
+        # ]
 
     def test_get_model_matrix(self, formula_expr, formula_exprs, data):
         mm_expr = formula_expr.get_model_matrix(data)