Normalize list-selection order to configuration order across every holder

Author: gupichon-soleil

pyaml/common/holders/diagnostic_holder.py

@@ -124,8 +124,8 @@ def __getitem__(self, key: str | list[str] | tuple[str, ...]) -> "Element | Elem
         store = self._peer._DIAG
         if isinstance(key, str) and not key.startswith(("re:", "~")) and not is_wildcard(key):
             return self._peer._get_diagnostic(key)
-        names = resolve_names(store.keys(), key, what="Diagnostic")
-        return ElementArray("", [store[n] for n in names])
+        matched = set(resolve_names(store.keys(), key, what="Diagnostic"))
+        return ElementArray("", [v for n, v in store.items() if n in matched])
 
     @property
     def betatron_tune(self) -> BetatronTuneMonitor:

pyaml/common/holders/element_holder.py

@@ -336,7 +336,8 @@ def find_elements(self, filter: str | list[str] | tuple[str, ...]) -> list[str]:
             If a literal pattern (or a ``~``-prefixed literal) matches no
             element, or a ``re:`` pattern is not a valid regular expression.
         """
-        return resolve_names(self._ALL.keys(), filter, what="Element")
+        matched = set(resolve_names(self._ALL.keys(), filter, what="Element"))
+        return [n for n in self._ALL if n in matched]
 
     def _fill_array(
         self,
@@ -518,14 +519,14 @@ def __getitem__(self, key: int | slice | str | list[str] | tuple[str, ...]) -> E
         """
         if isinstance(key, str):
             if key.startswith("re:") or key.startswith("~") or is_wildcard(key):
-                names = resolve_names(self._ALL.keys(), key)
-                return self.get()._typed_array([self._ALL[n] for n in names])
+                matched = set(resolve_names(self._ALL.keys(), key))
+                return self.get()._typed_array([v for n, v in self._ALL.items() if n in matched])
             if key not in self._ALL:
                 raise PyAMLException(f"Element {key} not defined")
             return self._ALL[key]
         if isinstance(key, (list, tuple)):
-            names = resolve_names(self._ALL.keys(), key)
-            return self.get()._typed_array([self._ALL[n] for n in names])
+            matched = set(resolve_names(self._ALL.keys(), key))
+            return self.get()._typed_array([v for n, v in self._ALL.items() if n in matched])
         if isinstance(key, int):
             return list(self._ALL.values())[key]
         if isinstance(key, slice):

pyaml/common/holders/rf_holder.py

@@ -80,8 +80,8 @@ def __getitem__(self, key: str | list[str] | tuple[str, ...]) -> "RFTransmitter
         store = self._peer._RFTRANSMITTER
         if isinstance(key, str) and not key.startswith(("re:", "~")) and not is_wildcard(key):
             return self._peer._get("RFTransmitter", key, store)
-        names = resolve_names(store.keys(), key, what="RFTransmitter")
-        return ElementArray("", [store[n] for n in names])
+        matched = set(resolve_names(store.keys(), key, what="RFTransmitter"))
+        return ElementArray("", [v for n, v in store.items() if n in matched])
 
     def add(self, rf: RFTransmitter):
         """
@@ -195,8 +195,8 @@ def __getitem__(self, key: str | list[str] | tuple[str, ...]) -> "RFPlant | Elem
         store = self._peer._RFPLANT
         if isinstance(key, str) and not key.startswith(("re:", "~")) and not is_wildcard(key):
             return self._peer._get("RFPlant", key, store)
-        names = resolve_names(store.keys(), key, what="RFPlant")
-        return ElementArray("", [store[n] for n in names])
+        matched = set(resolve_names(store.keys(), key, what="RFPlant"))
+        return ElementArray("", [v for n, v in store.items() if n in matched])
 
     def add(self, rf: RFPlant):
         """

pyaml/common/holders/tool_holder.py

@@ -115,8 +115,8 @@ def __getitem__(self, key: str | list[str] | tuple[str, ...]) -> "Element | Elem
         store = self._peer._TOOLS
         if isinstance(key, str) and not key.startswith(("re:", "~")) and not is_wildcard(key):
             return self._peer._get_tool(key)
-        names = resolve_names(store.keys(), key, what="Tool")
-        return ElementArray("", [store[n] for n in names])
+        matched = set(resolve_names(store.keys(), key, what="Tool"))
+        return ElementArray("", [v for n, v in store.items() if n in matched])
 
     def _validate_type(self, name: str, obj: Element, expected_type: type) -> Element:
         """

tests/common/test_array_holder_navigation.py

@@ -116,11 +116,17 @@ def test_configured_exclusion_family_reproduced_with_selection_and_difference():
 
 
 def test_configured_exclusion_family_reproduced_in_a_single_getitem_call():
-    """The YAML `elements:` selector list (`[QD2*, QF1*, ~QF1E-C05, ~Q???-C06]`) works verbatim through `[]`.
+    """The YAML `elements:` selector list (`[QD2*, QF1*, ~QF1E-C05, ~Q???-C06]`) selects the
+    same *family* of elements through `[]` as declared for `QForTest`.
 
     Selection goes through the top-level holder, not `.magnets`, so it resolves patterns
     against the same global element pool `find_elements()`/`_fill_array` use to build
-    `QForTest` in the YAML, and so lands in the same order.
+    `QForTest` in the YAML. The two therefore agree on *which* elements are selected, but not
+    necessarily on their order: `_fill_array` concatenates matches pattern by pattern (all
+    `QD2*` matches, then all `QF1*` matches) to build the named array, while `[]` (like every
+    other list-selection accessor, see `test_every_accessor_returns_a_list_selection_in_configuration_order`)
+    normalizes its result to configuration order regardless of how many patterns were
+    involved or in what order they matched.
     """
     sr = Accelerator.load("tests/config/EBSTune-patterns.yaml", ignore_external=True)
     sr.design.get_lattice().disable_6d()
@@ -129,7 +135,8 @@ def test_configured_exclusion_family_reproduced_in_a_single_getitem_call():
     reproduced = sr.design[["QD2*", "QF1*", "~QF1E-C05", "~Q???-C06"]]
 
     assert type(reproduced) is type(q_for_test)
-    assert reproduced == q_for_test
+    assert set(reproduced.names()) == set(q_for_test.names())
+    assert reproduced.names() == [n for n in sr.design.get().names() if n in set(q_for_test.names())]
 
 
 def test_array_getitem_supports_a_negated_regex_pattern():
@@ -193,7 +200,9 @@ def test_magnet_holder_getitem_wildcard_returns_a_magnet_array(holder):
 def test_magnet_and_magnets_holder_getitem_agree_on_order(holder):
     """.magnet (singular) and .magnets (plural) must return the same elements in the same
     order for the same name list, regardless of the order the names were requested in. The
-    reference order is the configuration order, not the order of the requested names."""
+    reference order is the configuration order, not the order of the requested names. This
+    must also hold for the top-level holder itself (`[]` and `find_elements`), not just its
+    typed sub-holders."""
     forward = holder.magnet[["SH1A-C01-H", "SH1A-C01-V"]]
     backward = holder.magnet[["SH1A-C01-V", "SH1A-C01-H"]]
     assert forward.names() == backward.names() == ["SH1A-C01-H", "SH1A-C01-V"]
@@ -202,6 +211,33 @@ def test_magnet_and_magnets_holder_getitem_agree_on_order(holder):
     backward_plural = holder.magnets[["SH1A-C01-V", "SH1A-C01-H"]]
     assert forward_plural.names() == backward_plural.names() == forward.names()
 
+    forward_top_level = holder[["SH1A-C01-H", "SH1A-C01-V"]]
+    backward_top_level = holder[["SH1A-C01-V", "SH1A-C01-H"]]
+    assert forward_top_level.names() == backward_top_level.names() == forward.names()
+
+    assert holder.find_elements(["SH1A-C01-H", "SH1A-C01-V"]) == forward.names()
+    assert holder.find_elements(["SH1A-C01-V", "SH1A-C01-H"]) == forward.names()
+
+
+def test_combined_function_magnet_and_bpm_holders_return_list_selections_in_configuration_order(holder):
+    """Every typed sub-holder must normalize list selections to configuration order, not just
+    .magnet/.magnets: this must also hold for .combined_function_magnet(s) and .bpm(s)."""
+    cfm_forward = holder.combined_function_magnet[["SH1A-C01", "SH1A-C02"]]
+    cfm_backward = holder.combined_function_magnet[["SH1A-C02", "SH1A-C01"]]
+    assert cfm_forward.names() == cfm_backward.names() == ["SH1A-C01", "SH1A-C02"]
+
+    cfm_forward_plural = holder.combined_function_magnets[["SH1A-C01", "SH1A-C02"]]
+    cfm_backward_plural = holder.combined_function_magnets[["SH1A-C02", "SH1A-C01"]]
+    assert cfm_forward_plural.names() == cfm_backward_plural.names() == cfm_forward.names()
+
+    bpm_forward = holder.diagnostic.bpm[["BPM_C04-01", "BPM_C04-02"]]
+    bpm_backward = holder.diagnostic.bpm[["BPM_C04-02", "BPM_C04-01"]]
+    assert bpm_forward.names() == bpm_backward.names() == ["BPM_C04-01", "BPM_C04-02"]
+
+    bpm_forward_plural = holder.diagnostic.bpms[["BPM_C04-01", "BPM_C04-02"]]
+    bpm_backward_plural = holder.diagnostic.bpms[["BPM_C04-02", "BPM_C04-01"]]
+    assert bpm_forward_plural.names() == bpm_backward_plural.names() == bpm_forward.names()
+
 
 @pytest.mark.parametrize(
     "select",
@@ -214,12 +250,29 @@ def test_magnet_and_magnets_holder_getitem_agree_on_order(holder):
     ],
 )
 @pytest.mark.parametrize("reverse", [False, True], ids=["forward", "reversed"])
-def test_every_accessor_returns_a_list_selection_in_configuration_order(holder, select, reverse):
+@pytest.mark.parametrize(
+    "key, expected_members",
+    [
+        pytest.param(
+            ["SH1A-C01-H", "SH1A-C01-V", "SH1A-C02-H"],
+            {"SH1A-C01-H", "SH1A-C01-V", "SH1A-C02-H"},
+            id="literal-names",
+        ),
+        pytest.param(
+            ["SH1A-C0?-H", "SH1A-C01-V"],
+            {"SH1A-C01-H", "SH1A-C02-H", "SH1A-C01-V"},
+            id="wildcard-and-literal",
+        ),
+    ],
+)
+def test_every_accessor_returns_a_list_selection_in_configuration_order(holder, select, reverse, key, expected_members):
     """Every name-based accessor must return a list-of-patterns selection in configuration
-    order, whatever the order of the requested names, so that positional values line up the
-    same way whichever accessor built the array."""
-    key = ["SH1A-C01-H", "SH1A-C01-V", "SH1A-C02-H"]
-    expected = [n for n in holder.get().names() if n in key]
+    order, whatever the order of the requested names/patterns, so that positional values line
+    up the same way whichever accessor built the array. This must hold both for a list of
+    literal names and for a list mixing a wildcard (matching several names) with a literal
+    name, since a wildcard entry is where a naive pattern-order passthrough diverges from
+    configuration order the most visibly."""
+    expected = [n for n in holder.get().names() if n in expected_members]
     assert select(holder, key[::-1] if reverse else key) == expected
 
 

tests/common/test_element_holder_collection.py

@@ -226,8 +226,9 @@ def test_invalid_regex_raises_pyaml_exception(holder):
 
 
 def test_list_of_patterns_unions_results(holder):
+    """Order follows configuration (registration) order, not the order patterns were requested in."""
     selected = holder[["BPM_C04-01", "SH1A-C0?-H"]]
-    assert selected.names() == ["BPM_C04-01", "SH1A-C01-H", "SH1A-C02-H"]
+    assert selected.names() == ["SH1A-C01-H", "SH1A-C02-H", "BPM_C04-01"]
 
 
 def test_list_of_patterns_with_missing_literal_raises(holder):
@@ -253,20 +254,21 @@ def test_find_elements_supports_regex(holder):
 
 
 def test_find_elements_supports_a_list_of_patterns(holder):
-    assert holder.find_elements(["BPM_C04-01", "SH1A-C0?-H"]) == ["BPM_C04-01", "SH1A-C01-H", "SH1A-C02-H"]
+    """Order follows configuration (registration) order, not the order patterns were requested in."""
+    assert holder.find_elements(["BPM_C04-01", "SH1A-C0?-H"]) == ["SH1A-C01-H", "SH1A-C02-H", "BPM_C04-01"]
 
 
 def test_find_elements_supports_an_exclusion_in_a_list(holder):
     both_planes = holder.find_elements(["SH1A-C0?-H", "SH1A-C0?-V"])
-    assert both_planes == ["SH1A-C01-H", "SH1A-C02-H", "SH1A-C01-V", "SH1A-C02-V"]
+    assert both_planes == ["SH1A-C01-H", "SH1A-C01-V", "SH1A-C02-H", "SH1A-C02-V"]
 
     minus_one = holder.find_elements(["SH1A-C0?-H", "SH1A-C0?-V", "~SH1A-C02-V"])
-    assert minus_one == ["SH1A-C01-H", "SH1A-C02-H", "SH1A-C01-V"]
+    assert minus_one == ["SH1A-C01-H", "SH1A-C01-V", "SH1A-C02-H"]
 
 
 def test_getitem_supports_an_exclusion_in_a_list(holder):
     selected = holder[["SH1A-C0?-H", "SH1A-C0?-V", "~SH1A-C02-V"]]
-    assert selected.names() == ["SH1A-C01-H", "SH1A-C02-H", "SH1A-C01-V"]
+    assert selected.names() == ["SH1A-C01-H", "SH1A-C01-V", "SH1A-C02-H"]
 
 
 def test_getitem_lone_exclusion_pattern_means_everything_except(holder):

tests/diagnostics/test_diagnostic_accessors.py

@@ -1,3 +1,5 @@
+import copy
+
 import pytest
 
 from pyaml.accelerator import Accelerator
@@ -114,6 +116,27 @@ def test_diagnostic_getitem_wildcard_returns_an_array():
     assert design.diagnostic["MISSING*"].names() == []
 
 
+def test_diagnostic_getitem_list_selection_ignores_request_order():
+    """A list-of-patterns selection lands in configuration order, whatever order the names
+    were requested in, same as every other holder (see
+    test_every_accessor_returns_a_list_selection_in_configuration_order). Only one diagnostic
+    is registered by any existing fixture, so a distinct second entry is poked directly into
+    the store (a renamed copy of the existing monitor) to make request order and
+    configuration order observably different."""
+    design = Accelerator.load(
+        "tests/config/EBSOrbit.yaml",
+        ignore_external=True,
+        include_locations=False,
+    ).design
+    spare_monitor = copy.copy(design.get_betatron_tune_monitor("BETATRON_TUNE"))
+    spare_monitor._name = "SPARE_BETATRON_TUNE"
+    design._DIAG["SPARE_BETATRON_TUNE"] = spare_monitor
+
+    forward = design.diagnostic[["BETATRON_TUNE", "SPARE_BETATRON_TUNE"]]
+    backward = design.diagnostic[["SPARE_BETATRON_TUNE", "BETATRON_TUNE"]]
+    assert forward.names() == backward.names() == ["BETATRON_TUNE", "SPARE_BETATRON_TUNE"]
+
+
 def test_diagnostic_bpm_getitem_exact_name_matches_get():
     design = Accelerator.load(
         "tests/config/EBSOrbit.yaml",

tests/rf/test_rf_holder.py

@@ -1,3 +1,5 @@
+import copy
+
 import pytest
 
 from pyaml.accelerator import Accelerator
@@ -43,6 +45,31 @@ def test_rf_transmitter_holder_getitem_list_of_patterns(design):
     assert selected.names() == ["RFTRA1", "RFTRA2"]
 
 
+def test_rf_transmitter_holder_getitem_list_selection_ignores_request_order(design):
+    """A list-of-patterns selection lands in configuration order, whatever order the names
+    were requested in, same as every other holder (see
+    test_every_accessor_returns_a_list_selection_in_configuration_order)."""
+    forward = design.rf.transmitter[["RFTRA1", "RFTRA2"]]
+    backward = design.rf.transmitter[["RFTRA2", "RFTRA1"]]
+    assert forward.names() == backward.names() == ["RFTRA1", "RFTRA2"]
+
+
+def test_rf_holder_getitem_list_selection_ignores_request_order(design):
+    """Same configuration-order guarantee as the RF transmitter holder, at the RF plant level.
+
+    Only one RF plant is registered by this fixture, so a distinct second entry is poked
+    directly into the store (a renamed copy of the existing plant) to make request order and
+    configuration order observably different.
+    """
+    spare_plant = copy.copy(design.rf.get("DEFAULT_RF_PLANT"))
+    spare_plant._name = "SPARE_RF_PLANT"
+    design._RFPLANT["SPARE_RF_PLANT"] = spare_plant
+
+    forward = design.rf[["DEFAULT_RF_PLANT", "SPARE_RF_PLANT"]]
+    backward = design.rf[["SPARE_RF_PLANT", "DEFAULT_RF_PLANT"]]
+    assert forward.names() == backward.names() == ["DEFAULT_RF_PLANT", "SPARE_RF_PLANT"]
+
+
 def test_rf_transmitter_holder_getitem_regex(design):
     matching = design.rf.transmitter["re:^RFTRA[12]$"]
     assert sorted(matching.names()) == ["RFTRA1", "RFTRA2"]

tests/tuning_tools/test_tool_holder.py

@@ -33,6 +33,15 @@ def test_tool_holder_getitem_list_of_patterns(design):
     assert selected.names() == ["DEFAULT_TUNE_CORRECTION", "DEFAULT_ORBIT_CORRECTION"]
 
 
+def test_tool_holder_getitem_list_selection_ignores_request_order(design):
+    """A list-of-patterns selection lands in configuration order, whatever order the names
+    were requested in, same as every other holder (see
+    test_every_accessor_returns_a_list_selection_in_configuration_order)."""
+    forward = design.tool[["DEFAULT_TUNE_CORRECTION", "DEFAULT_ORBIT_CORRECTION"]]
+    backward = design.tool[["DEFAULT_ORBIT_CORRECTION", "DEFAULT_TUNE_CORRECTION"]]
+    assert forward.names() == backward.names() == ["DEFAULT_TUNE_CORRECTION", "DEFAULT_ORBIT_CORRECTION"]
+
+
 def test_tool_holder_getitem_regex(design):
     matching = design.tool["re:^DEFAULT_(TUNE|ORBIT)_CORRECTION$"]
     assert sorted(matching.names()) == ["DEFAULT_ORBIT_CORRECTION", "DEFAULT_TUNE_CORRECTION"]