Repository navigation
pyaml user feedback using SOLEIL II digital twin #29
Description
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
pyamlcodebase.
General Comments
- Coding Style:
- The codebase uses many
get/setfunctions, which feels un-Pythonic. Consider using properties with getters/setters for a smoother user experience.
- The codebase uses many
- 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).
- Function Naming:
- The function
pyaml(used to load.yamlconfigurations) should have a descriptive name, e.g.,load_pymal_config.
- The function
Specific Feedback
pyaml.lattice.simulator.Simulator / pyaml.controlsystem.TangoControlSystem
get_all_magnetsMethod:- Currently returns a
dict, but should return an array to enable filtering/sorting. - Question: Should this be a property instead of a method?
- Currently returns a
- Feature Request:
- Add a property/method to retrieve all elements (magnets, BPMs, devices, etc.), not just magnets.
pyaml.lattice.magnet
__repr__Method:- Should include the
control_mode(e.g.,simulatedvs.live) to avoid confusion.
- Should include the
__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', ...)
- Should print detailed information about the magnet, e.g.:
hardware_nameAccessibility:- 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'
- Control System Layer Access:
- Should provide easy access to the Python control system layer (e.g.,
pyTango/pyEPICS). - For Tango, suggest adding a
device_proxyattribute to return theTangoDeviceProxyfor the element.
- Should provide easy access to the Python control system layer (e.g.,
pyaml.arrays.magnet_arrays.MagnetArray
- Subset Handling:
- Subsets (e.g.,
qcorr[0:5]) should return aMagnetArray, not alist.- Bug:
qcorr[0:5].strengths→AttributeError: 'list' object has no attribute 'strengths'.
- Bug:
- Subsets (e.g.,
- The
MagnetArrayclass is currently too simplistic and lacks key features for practical use.- Support heterogeneous
MagnetArray(different magnet types). - Implement element access by:
hardware_namename- Element type filtering
- Support heterogeneous
pyaml.control.abstract_impl.RWStrengthScalar
getMethod:- Currently returns an array instead of a
float.- Example:
quad.strength.get() # Returns [0.0]
- Example:
- Currently returns an array instead of a
pyaml.arrays.magnet_array.RWMagnetStrength
- 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
- Bug:
- Should support setting RWArrays using a single value (not just arrays of the same shape).
Activity
JeanLucPons commentedon Oct 20, 2025
Many thanks for this feedback.
The .set() methods are there use to avoid:
In order to get rid of this kind on invisible error:
GamelinAl commentedon Oct 20, 2025
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 commentedon Oct 21, 2025
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 ?
__repr__/__str__stuff (this does not concern only magnets)Thanks
gubaidulinvadim commentedon Oct 21, 2025
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 commentedon Oct 22, 2025
Discussions are tied to a repository. The organizational level discussions I have set up to connect to the discussions in the
governancerepository 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
accmlrepository and put our feedback in that repository in the same way.JeanLucPons commentedon Oct 22, 2025
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 commentedon Oct 22, 2025
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 theget(in their case it's calledread) andsetin 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 dofrequency.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 domasterclock.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 commentedon Oct 22, 2025
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 commentedon Oct 22, 2025
OK, I will enable the issues in governance and move a discussion there (effectively to where all the discussions are)