pyaml user feedback using SOLEIL II digital twin

[GamelinAl]

Context

Thanks to the work of @Pichon, I was able to test the current pyaml version on the SOLEIL II digital twin (based on a modified version of the ESRF-EBS simulator).

Here is some user feedback on the current implementation, I propose to discuss it and them to split it in subtasks when we agree on some changes.

For this feedback, I focused on the most basic features of pyAML but I was also able to run the tune response matrix example of @JeanLucPons adapted to the SOLEIL II lattice. I think that with a few modifications to the actual code base, we will not be too far from a version in which we will be able to implement the chosen use cases.

• Version tested: Merge of 30 modular linking between pyat and pyaml elements pyaml#32, commit be8b5d0e5f2e08f674c7072d3761fd6f6f77bef2
• Background: Limited prior knowledge of the pyaml codebase.

---

General Comments

1. Coding Style:
   • The codebase uses many get/set functions, which feels un-Pythonic. Consider using properties with getters/setters for a smoother user experience.
2. Internal vs. User-facing Methods:
   • Clearly distinguish between methods intended for users and internal ones.
   • Internal methods should be prefixed with _ or __ to hide them from introspection (e.g., in IPython/terminal).
3. Function Naming:
   • The function pyaml (used to load .yaml configurations) should have a descriptive name, e.g., load_pymal_config.

---

Specific Feedback

pyaml.lattice.simulator.Simulator / pyaml.controlsystem.TangoControlSystem

1. get_all_magnets Method:
   • Currently returns a dict, but should return an array to enable filtering/sorting.
   • Question: Should this be a property instead of a method?
2. Feature Request:
   • Add a property/method to retrieve all elements (magnets, BPMs, devices, etc.), not just magnets.

---

pyaml.lattice.magnet

1. __repr__ Method:
   • Should include the control_mode (e.g., simulated vs. live) to avoid confusion.
2. __str__ Method:
   • Should print detailed information about the magnet, e.g.:

     Sextupole(name='SXF2', strength=809.008, currents=[140,...], hardware_name='AN01-AR/EM/SCF.02', control_mode='live', ...)

3. hardware_name Accessibility:
   • Should be accessible as a property (at least in "live" mode).
   • Current Workaround:

     quad.model.get_devices()[0].name()  # Returns 'AN14-AR/EM/OH.03-CQLN.13/strength'

4. Control System Layer Access:
   • Should provide easy access to the Python control system layer (e.g., pyTango/pyEPICS).
   • For Tango, suggest adding a device_proxy attribute to return the TangoDeviceProxy for the element.

---

pyaml.arrays.magnet_arrays.MagnetArray

1. Subset Handling:
   • Subsets (e.g., qcorr[0:5]) should return a MagnetArray, not a list.
     • Bug: qcorr[0:5].strengths → AttributeError: 'list' object has no attribute 'strengths'.
2. The MagnetArray class is currently too simplistic and lacks key features for practical use.
   • Support heterogeneous MagnetArray (different magnet types).
   • Implement element access by:
     • hardware_name
     • name
     • Element type filtering

---

pyaml.control.abstract_impl.RWStrengthScalar

1. get Method:
   • Currently returns an array instead of a float.
     • Example:

       quad.strength.get()  # Returns [0.0]

---

pyaml.arrays.magnet_array.RWMagnetStrength

1. Setting Values:
   • Should support setting RWArrays using a single value (not just arrays of the same shape).
     • Bug: sh1.strengths.set(1) → TypeError: 'float' object is not subscriptable

---

[JeanLucPons]

Many thanks for this feedback.

• I think few bug (and improvement) you mention are already fixed in the last release.
• I confirm that a good refurbishment is needed to have more appropriate names and make private fields when possible.
• Concerning getter and setter, could you be more precise ?
  The .set() methods are there use to avoid:

quad.strength = 0.5

In order to get rid of this kind on invisible error:

quad.strenght = 0.5

[GamelinAl]

You're wellcome, it's good to be finally able to try it. My idea is too give this kind of feedback every two week or so (or more often if needed) to test what has been developped.

For the setter and getter, it's just a comment related to my preferences as a user. To me, it's more natural python code, more direct, compact and it has a better readability. When you learn a new python package, it's much more simple if it works like the rest of the python ecosystem.
For the kind of error you mention, with code completion it does not happen so often I would say.

[JeanLucPons]

Would it be possible to move this overall feedback to the discussion section and cut into several issues in order to let people take some tasks ?

• Magnet arrays
• __repr__ / __str__ stuff (this does not concern only magnets)
• Having shortcut to control system (should be done inside the backend, no specific CS code in the core)
• getter/setter (if we agree, personally i dislike, i already lost lots of time and in pytango we regret to have this)

Thanks

[GamelinAl]

We should discuss the pro and cons of using or not using getter/setter or set/get methods.

Could you expend on your experiance with pytango and why you regret to have this implemented? It's actually one of the things I like about pytango as a user, it's very user friendly if you are used to python.

For exemple, in the current implementation of the Magnet class, I think it would be better to have a name property protected against writing instead of get_name method.

[JeanLucPons]

OK for name as property instead of get_name() as it is well defined.

For getting all magnets, for getting the rf_plant, etc we will add fully typed properties when possible.

Concerning pytango, It took me age to find a bug in the RF software where ds.Votlage = was wrote instead of ds.Voltage = and of course the Voltage attribute was not written and no error was triggered as it is a fully valid python code. Now I always use write_attribute().

I don't want to impose something. It just my humble opinion. But this kind of writing is more compact for sure but can lead to error very difficult to find. For the story, we have even disabled python dynamic attribute creation in the DeviceProxy class to prevent this kind of error but it totally broke BpmDevice() class (from @swhite2401) which inherits from DeviceProxy so we reverted this modification.

[gubaidulinvadim]

I've tried to move this issue to discussion. There's a technical problem with GitHub. I can only move it to discussion of the pyaml repository but not to the discussion of pyAML organisation (where all discussions are). In the pyAML organisation, I cannot find a way to add issues. @TeresiaOlsson maybe you have more rights than me and can enable issues in the organisation. Then we can convert some of the issues, like feedback from SOLEIL and MAX IV, to discussions instead.

[TeresiaOlsson]

Discussions are tied to a repository. The organizational level discussions I have set up to connect to the discussions in the governance repository to make them general. Issues doesn't seem to exist on the organizational level in Github.

But why do you want to move it when the feedback is related to this repository? I put the feedback from HZB and MAX IV explicitly in this repository for that reason and not as a discussion. In a couple of weeks Marco and I will also test the accml repository and put our feedback in that repository in the same way.

[gubaidulinvadim]

It is to have all of the discussion in one place. Issues are used for the to-do list. For the moment I only look on discussions and project tabs to follow the development. It is not convenient to go into issues tab and search for issues that are not attached to the project.

[JeanLucPons]

The problem is that issues appear in the project TODO list.
We can of course split the feedback into several issues and keep the main discussion in the TODO list.
But it would be better if the overall feedback appears only in the discussion of this repository.

[TeresiaOlsson]

For the setter and getter, it's just a comment related to my preferences as a user. To me, it's more natural python code, more direct, compact and it has a better readability. When you learn a new python package, it's much more simple if it works like the rest of the python ecosystem. For the kind of error you mention, with code completion it does not happen so often I would say.

I agree and I already brought this point up during the specification writing process. However, other packages (such as ophyd-async) I think have managed to introduce the get (in their case it's called read) and set in the way Jean-Luc wants without it feeling strange for the users.

They way they have done it is by introducing a signal level (which also actually is a device since it inherits from device) and have an architecture which is very clear for the users where they understand that all attributes are either devices or signals. Then for me it never has felt strange that I need to do frequency.read() because I know it's not just a simple attribute but an object that needs to connect to the correct signal backend (epics, tango, mock etc). In addition to that they also have the same interface on the device level. So I can do masterclock.read() and it will read all the attributes in that device. That I also think helps to justify it for the users since they can use the same commands at all levels independently of how many layers of devices one device is built up of.

[TeresiaOlsson]

The problem is that issues appear in the project TODO list. We can of course split the feedback into several issues and keep the main discussion in the TODO list. But it would be better if the overall feedback appears only in the discussion of this repository.

Okay. To move it to the discussion in this repository sounded like @gubaidulinvadim could do? It's only moving to the organizational level discussion that didn't work?

[gubaidulinvadim]

This issue now has been converted to a discussion. Next actions to take:

• Create issues based on the feedback presented here

[GamelinAl]

I have created the following issues to divide the work on points which, I think, we all agree must be done:

• Improve MagnetArray class and implement generic ElementArray class pyaml#41
• String representation of pyAML objects pyaml#42
• Feature: add control system access to pyAML elements pyaml#44
• Feature: Add hardware_name and model_name to elements pyaml#46