Elementholder api refurbishment

[gupichon]

Description

Integration PR for epic #199, ElementHolder API refurbishment. Aggregates the sub-features that split the historically monolithic ElementHolder into typed sub-holders (.magnet(s), .bpm(s), .rf, .diagnostic, .tool, ...) reachable through Python properties, with typed get() and array access instead of the old untyped get_all*() and get_*s() list-returning methods.

This branch is merged incrementally as each sub-issue lands, rather than opened as a single large PR, to keep review scoped per sub-feature.

Related Issue

• Tracks ElementHolder API refurbishment #199

Sub-issues and their status on this branch:

• Feature: Add generic ElementHolder collection access #366, add generic ElementHolder collection access. Closed, merged to main before this branch was cut.
• Feature: Complete magnet holder navigation #373, complete magnet holder navigation. Merged via Complete magnet holder navigation (#373) #426, get_cfm() shortcut and dynamic attribute access on GenericArrayHolder based holders.
• Feature: Add a public diagnostic holder #372, add a public diagnostic holder. Not yet started.
• Feature: Add an RF masterclock facade #374, add an RF masterclock facade. PR Add an RF masterclock facade #427 was closed without merging. Reviewers flagged that masterclock.voltage is conceptually wrong, voltage needs its own facade, a cavity concept was proposed, instead of hanging off the default RF plant. Re-opening this requires a new design, not a resubmission of Add an RF masterclock facade #427's diff.
• Feature: Add a public tuning and measurement tool holder #375, add a public tuning and measurement tool holder. Implemented, ToolHolder, tool.get(name=None), typed tune, trm, orbit, orm, chromaticity, crm, dispersion properties with type validation. Tests added, full suite green. PR not yet opened.
• Feature: Unify name/wildcard access and miss-behavior across all holders #433, unify name and wildcard access and miss behavior across all holders, new issue. Surfaced while implementing Feature: Add a public tuning and measurement tool holder #375, sr.live.bpm[...] has no wildcard support at all, and every holder disagrees on what happens when a name is missing.

Changes to existing functionality

• None beyond what each merged sub-issue's own PR already documents, see Complete magnet holder navigation (#373) #426 for Feature: Complete magnet holder navigation #373, and Feature: Add a public tuning and measurement tool holder #375's own PR for the ToolHolder aliasing. This PR does not itself change behavior, it only aggregates already reviewed sub-PRs.

Testing

No tests added directly on this integration branch, each sub-issue's PR carries its own tests, see #426 for #373's tests/common/test_array_holder_navigation.py, and #375's PR for tests/tuning_tools/test_tool_accessors.py.

Verify that your checklist complies with the project

• New and existing unit tests pass locally
• Tests were added to prove that all features and changes are effective, covered per sub-PR
• The code is commented where appropriate, covered per sub-PR
• Any existing features are not broken

[JeanLucPons]

If bpm(s) moves to diagnostic holder, bpm.py should ne moved in diagnostics directory.
and empy bpm folder removed.

[gupichon]

If bpm(s) moves to diagnostic holder, bpm.py should ne moved in diagnostics directory. and empy bpm folder removed.

Yes, but @gubaidulinvadim suggested doing it later in another PR. Would you prefer to do it in this one? Personally, I don't mind.

[gupichon]

Everything has been merged here. I will rebase this afternoon and request a review.

@JeanLucPons, @TeresiaOlsson, @GamelinAl, @simoneliuzzo, @kparasch, @gubaidulinvadim and anyone else: feel free to start reviewing right now, as things won't change much after the rebase.

[kparasch]

Very minor comment. Could we simplify with:

return self._validate_type(name, self.get(name), Tune)

?

[kparasch]

I tend to prefer breaking backward-compatibility this early in development.

If we say that sr.live.tool.orm is the preferred way to access the orm tool, we should enforce it.

[kparasch]

Looks ok to me, some minor comments only

[GamelinAl]

Two issues:

1. Compared to #199 we still miss the support of ElementHolder get method for named arrays: sr.live.get("CELL08"). Only the old syntax sr.live.get_elements("CELL08") is working.
2. We should also clean the API, I would say we can remove:
   • get_element
   • get_elements
   • get_all_elements
   • get_*_tuning
   • get_betatron_tune_monitor

After this it should be good I think.

[gupichon]

Finally ready for review!

[JeanLucPons]

If bpm(s) moves to diagnostic holder, bpm.py should ne moved in diagnostics directory. and empy bpm folder removed.

bpm.py is still in bpm folder ?

[gupichon]

If bpm(s) moves to diagnostic holder, bpm.py should ne moved in diagnostics directory. and empy bpm folder removed.

bpm.py is still in bpm folder ?

I have already replied, maybe you missed my message or I missed your answer:

Yes, but @gubaidulinvadim suggested doing it later in another PR. Would you prefer to do it in this one? Personally, I don't mind.

[GamelinAl]

IMO, we could do it here so it's done.

Two more things:

1. sr.design["QF1E-C04"] and sr.design.magnet["QF1E-C04"] return a Quadrupole but sr.design.magnets["QF1E-C04"] returns a one element MagnetArray. It would be good to have consistency on this.
2. The error when the config is wrong is less clear than before. For example with config:

  - type: pyaml.arrays.magnet
    name: QuadsCell06
    elements:
      - QF1A-C06
      - QF1E-C6      # typo

The error is now: Element QF1E-C6 not defined
Before it was: MagnetArray QuadsCell06 : Magnet QF1E-C6 not defined @index 1

Sorry it's a bit beyond this PR but I can't help myself to see that now having both sr.design.magnet and sr.design.magnets is very redondant for users. Before this PR the split made sense: magnet looked up single magnets and magnets looked up families.

With this PR, sr.design.magnets can do everything that sr.design.magnet does. Internally we might need both (is this still true ?) but sr.design.magnet could be made private (sr.design._magnet) to simply the API for users.

The same reasoning applies to bpm/bpms, combined_function_magnet(s) and serialized_magnet(s).
Tell me if it's too much and I will make a new issue once this is merged.

[gupichon]

@GamelinAl, I agree with your proposal. I would like the others to confirm before starting development.

[TeresiaOlsson]

IMO, we could do it here so it's done.

Two more things:

1. `sr.design["QF1E-C04"]` and `sr.design.magnet["QF1E-C04"]` return a Quadrupole but `sr.design.magnets["QF1E-C04"]`  returns a one element MagnetArray. It would be good to have consistency on this.

2. The error when the config is wrong is less clear than before. For example with config:

  - type: pyaml.arrays.magnet
    name: QuadsCell06
    elements:
      - QF1A-C06
      - QF1E-C6      # typo

The error is now: Element QF1E-C6 not defined Before it was: MagnetArray QuadsCell06 : Magnet QF1E-C6 not defined @index 1

Sorry it's a bit beyond this PR but I can't help myself to see that now having both sr.design.magnet and sr.design.magnets is very redondant for users. Before this PR the split made sense: magnet looked up single magnets and magnets looked up families.

With this PR, sr.design.magnets can do everything that sr.design.magnet does. Internally we might need both (is this still true ?) but sr.design.magnet could be made private (sr.design._magnet) to simply the API for users.

The same reasoning applies to bpm/bpms, combined_function_magnet(s) and serialized_magnet(s). Tell me if it's too much and I will make a new issue once this is merged.

I think having both sr.design.magnets and sr.design.magnet is very confusing. First time I tried it I thought that maybe one of them was a typo until I realised that there was two APIs. And then I had to remember if what I was trying to get was magnet in plural or singular. If that API finally can be gone that would be great.

[kparasch]

I agree that sr.design.magnet is now redundant and can be confusing. Since we can have sr.design["QF1E-C04"] return a single quadrupole, it makes sense to make to have sr.design.magnets to fetch an array of magnets, and remove sr.design.magnet to avoid confusion.

From the #461 discussion, I think we might end up having to revise what sr.design.magnets can do, in the sense of which magnets should it be able to access (when we need to mix serialized and individually powered magnets).

So even though I support suppressing sr.design.magnet here, I think the #461 discussion may trigger a need for it come back.

[TeresiaOlsson]

I think sr.design.magnets is more complicated to type. So if one should be made redundant I think it might be better to keep sr.design.magnet and have it be able to return both single and plural magnets. Then the API is based on the type rather than if it's singular or plural. More similar to the API in pyAT where you can get elements by the type by doing something like ring[at.Dipole] which I think is nice and easy to use.

[JeanLucPons]

If bpm(s) moves to diagnostic holder, bpm.py should ne moved in diagnostics directory. and empy bpm folder removed.

bpm.py is still in bpm folder ?

I have already replied, maybe you missed my message or I missed your answer:

Yes, but @gubaidulinvadim suggested doing it later in another PR. Would you prefer to do it in this one? Personally, I don't mind.

I would prefer something coherent here. But It can be done later. As you wish.

[JeanLucPons]

1. sr.design["QF1E-C04"] and sr.design.magnet["QF1E-C04"] return a Quadrupole but sr.design.magnets["QF1E-C04"] returns a one element MagnetArray. It would be good to have consistency on this.

For me:
sr.design.magnets["QF1E-C04"] should fail is no array named QF1E-C04 is defined.

Personally, I think that:

sr.design[...] should return element array only as It was specified.

I recall that array and single element does not have the same signature, If you want to do all with a single method, typing will not allow to provide a good auto-competition. Having too generic function will be confusing for user.

[GamelinAl]

In the maintainer's meeting, we agreed to merge this as it is (or nearly).
We will open an issue to discuss how the API should evolve afterward (sr.design.magnets vs sr.design.magnet).