String representation

[gubaidulinvadim]

Work in progress!
Modified pyaml_repr to print nested string representation. For BPM, this gives better results.

sr = Accelerator.load("tests/config/EBSOrbit.yaml")
bpm_design = sr.design.get_bpm("BPM_C04-04")
print(bpm_design)
    BPM(description=None, lattice_names='BPM_C04-04', name='BPM_C04-04', peer=Simulator(name='design', lattice='/home/gubaidulin/codes/operation/pyAML/pyaml/tests/config/sr/lattices/ebs.mat', mat_key=None, linker=None, description=None))

There are a few issues still that can be resolved in different ways.

• There's no x_pos/y_pos (control system name string) in the string representation. It was present before.
  • We could make x_pos and y_pos 'public' by removing '_'. Then they would appear automatically in the string representation.
  • Another option is to make an 'include' keyword, similar to 'exclude' and explicitly include some attributes.
• I'm worried if nested string representation will not have the same issues as we had before with very long and unreadable string representation. Maybe there should be a length limit?

When solved resolves #345

[JeanLucPons]

Thanks !

Yes for very long representation we used pydantic feature param = Field(..., repr=False).
Now we have an exclude fields that we can use.
Concerning the Element peer, there was a specific case for it, we printed only Simulator('design') using method below:

attached_to(self) -> str:
        """
        Returns a string of which peer the element is attached to.
        """
        return "None" if self._peer is None else f"{self._peer.__class__.__name__}:{self._peer.name()}"

Concerning devices x_pos,..., I think we should remove the _.

[gubaidulinvadim]

@JeanLucPons @GamelinAl including properties like x_pos "publically" would also solve #44 and give control system access easily. For example (preudocode that may contain mistakes,

from tango import DeviceProxy
from pyaml.accelerator import Accelerator

sr = Accelerator.load('sr.pyaml')
tango_device = DeviceProxy(sr.live.get_bpm('BPM').x_pos.rsplit('/', 1))

[gubaidulinvadim]

In case of `public' x_pos, y_pos, tilt_name, x_offset, y_offset we also can remove the get_X_device functions, right?

[JeanLucPons]

I also remarked issue with LinearMagnetModel:

from pyaml.accelerator import Accelerator
sr = Accelerator.load("tests/config/EBSOrbit.yaml")
print(sr.design.magnet.get("QF6B-C04"))

Quadrupole(peer='Simulator:design', name='QF6B-C04', model_name='QF6B-C04', magnet_model=LinearMagnetModel)

I works better with IdentityCFMagnetModel but powerconverters (or physics) are not printed.

from pyaml.accelerator import Accelerator
sr = Accelerator.load("tests/config/EBSOrbit.yaml")
print(sr.design.magnet.get("SJ2E-C04-H"))

HCorrector(peer='Simulator:design', name='SJ2E-C04-H', model_name='SJ2E-C04', magnet_model=IdentityCFMagnetModel(multipoles=['B0', 'A0', 'A1', 'B2'], units=['rad', 'rad', 'm-1', 'm-2']))

[GamelinAl]

I think we should also aim to improve the string repr for our array-like elements.

This is usefull:

> sr.design.get_all_bpms()[0]
BPM(description=None, lattice_names='BPM_001', name='BPM_001', peer=Simulator(name='design', lattice='/home/gamelina/Python_dev/pyaml/examples/SOLEIL_examples/SOLEIL_II_V3631_sym1_V001_database_rf.m', mat_key=None, linker=<pyaml.lattice.attribute_linker.PyAtAttributeElementsLinker object at 0x7326891e6720>, description=None), tilt_name=None, x_offset=None, x_pos='AN01-SD/DG-EPOS/BPM.02/x_pos', y_offset=None, y_pos='AN01-SD/DG-EPOS/BPM.02/y_pos')

But currently the repr of the full array is just not useful. I would rather have something like:

> sr.design.get_all_bpms()[0]
[BPM('BPM_001'), BPM('BPM_002'), BPM('BPM_003'), ...]

Currently methods like sr.design.get_all_bpms return a list so it's not possible to do something custom but this will soon change with #199 if I am not mistaken. Then something more custom is possible. Do you think we need another issue to discuss this?

[JeanLucPons]

The new API:

sr.design.bpm.all() # Return a python list a all BPM
sr.design.bpms.get() # Return a BPMArray containing all BPM
sr.design.bpms.get("BPM") # Return a BPMArray named BPM from the config

For the moment both (list and array) print the same thing.

I agree we can print somehing like:
For a python list (a complete repr):

[BPM(peer=Simulator('design'), description=None, lattice_names='BPM_C04-04', name='BPM_C04-04'), ... ]

For Array:

BPMArray[peer=Simulator('design'), names=[BPM_C04-01', 'BPM_C04-02', 'BPM_C04-03',...]]

[gubaidulinvadim]

In fact, it's better to wait for @TeresiaOlsson to finish removing ConfigModel. Right now it's a bit painful to go through the string representation because some strings are printed from ConfigModel and not from new pyaml_repr. I will continue once ConfigModels are removed.

[TeresiaOlsson]

In fact, it's better to wait for @TeresiaOlsson to finish removing ConfigModel. Right now it's a bit painful to go through the string representation because some strings are printed from ConfigModel and not from new pyaml_repr. I will continue once ConfigModels are removed.

I like this. My modifications to the pyaml_repr was a bit of a quick fix to be able to replace how the repr is generated when there is no ConfigModel anymore but I already noticed that it isn't used consistently everywhere. There are definitely also improvements that could be done to the format so we can have a consistent way the string representation looks everywhere in the code.

I saw that you have finished reviewing and merging most of the PRs for removing the ConfigModels now :) I will be back at work tomorrow and then I will rebase the last ones and continue working on the documentation.

My hope was that we as part of the documentation somehow can call the repr automatically and more easily be able to spot the objects where a better string representation is needed. I think it would be nice if it somehow was printed as part of the API documentation. Then when you develop a new module and check the documentation which was automatically generated for it you will notice that the string representation isn't nice without having to run it manually. But I'm still working on the API documentation. I'm planning to start a copier template that can be applied to all repositories.

[TeresiaOlsson]

I think it depends on what you want. The idea was here: https://github.com/orgs/python-accelerator-middle-layer/discussions/52 to use _ to indicate that it's a private attribute that other classes or users shouldn't be allowed to access directly but only through properties. In that way a public interface is defined consisting of only simple public attributes and properties which can have more complex logic. And if it turns out that other classes need to access a private attribute, that would be an indication that there is something wrong with the interface and it needs to be modified.

In some cases I made a private attribute because the attribute is used in the constructor to initialize other things. So if I made it public the user could potentially change it but without triggering the required changes in other attributes. The fix for those cases is to add a property setter but I skipped doing it in these PRs to not make them too large. My thinking was that we more easily can add this later when we discover where it would be useful.

So my suggestion is to instead of making the attributes public (unless they are simple ones), add a property. Also read-only properties (without a setter) should automatically show up in the string representation otherwise it's a bug.

I think in general it is a nice idea that what shows up in the string representation is only the public interface of the class because it's a common way for the users to know which attributes are available for them to read and change.

[gubaidulinvadim]

I think it depends on what you want. The idea was here: https://github.com/orgs/python-accelerator-middle-layer/discussions/52 to use _ to indicate that it's a private attribute that other classes or users shouldn't be allowed to access directly but only through properties. In that way a public interface is defined consisting of only simple public attributes and properties which can have more complex logic. And if it turns out that other classes need to access a private attribute, that would be an indication that there is something wrong with the interface and it needs to be modified.

In some cases I made a private attribute because the attribute is used in the constructor to initialize other things. So if I made it public the user could potentially change it but without triggering the required changes in other attributes. The fix for those cases is to add a property setter but I skipped doing it in these PRs to not make them too large. My thinking was that we more easily can add this later when we discover where it would be useful.

So my suggestion is to instead of making the attributes public (unless they are simple ones), add a property. Also read-only properties (without a setter) should automatically show up in the string representation otherwise it's a bug.

I think in general it is a nice idea that what shows up in the string representation is only the public interface of the class because it's a common way for the users to know which attributes are available for them to read and change.

Yes, but the point is that x_pos and y_pos should be public. They can be used for direct access to underlying control system attributes, like requested in #44 . So, for most objects, we could provide some kind of control_system_names.

[TeresiaOlsson]

I think it depends on what you want. The idea was here: https://github.com/orgs/python-accelerator-middle-layer/discussions/52 to use _ to indicate that it's a private attribute that other classes or users shouldn't be allowed to access directly but only through properties. In that way a public interface is defined consisting of only simple public attributes and properties which can have more complex logic. And if it turns out that other classes need to access a private attribute, that would be an indication that there is something wrong with the interface and it needs to be modified.
In some cases I made a private attribute because the attribute is used in the constructor to initialize other things. So if I made it public the user could potentially change it but without triggering the required changes in other attributes. The fix for those cases is to add a property setter but I skipped doing it in these PRs to not make them too large. My thinking was that we more easily can add this later when we discover where it would be useful.
So my suggestion is to instead of making the attributes public (unless they are simple ones), add a property. Also read-only properties (without a setter) should automatically show up in the string representation otherwise it's a bug.
I think in general it is a nice idea that what shows up in the string representation is only the public interface of the class because it's a common way for the users to know which attributes are available for them to read and change.

Yes, but the point is that x_pos and y_pos should be public. They can be used for direct access to underlying control system attributes, like requested in #44 . So, for most objects, we could provide some kind of control_system_names.

Okay, then it sounds like a bug in the BPM class. I was conservative and made attributes private unless I was fully sure that it was something the user should be allowed to change.

[TeresiaOlsson]

I think the problem is what type self._x_pos is. If I look at the BPM class the doc string for the class says it's a string. And it's not used anywhere except in get_pos_devices where it according to the doc string of that function suddenly has turned into a DeviceAccess object. But that change is not happening inside the BPM class but somewhere else making the BPM class not self-contained. I think that was why I made it private because otherwise the user expects that they could change it to another string but instead of updating the DeviceAccess object it will overwrite it.

But maybe this is not true anymore after the changes to the controlsystem backend? Maybe self._x_pos is always a string now and never transformed into a DeviceAccess object so it's just the doc string that is wrong?

I think there were many places like this where I made an attribute private since it internally changed type from a string to something else and a property would be needed to handle changes to it correctly.

[gubaidulinvadim]

I think the problem is what type self._x_pos is. If I look at the BPM class the doc string for the class says it's a string. And it's not used anywhere except in get_pos_devices where it according to the doc string of that function suddenly has turned into a DeviceAccess object. But that change is not happening inside the BPM class but somewhere else making the BPM class not self-contained. I think that was why I made it private because otherwise the user expects that they could change it to another string but instead of updating the DeviceAccess object it will overwrite it.

But maybe this is not true anymore after the changes to the controlsystem backend? Maybe self._x_pos is always a string now and never transformed into a DeviceAccess object so it's just the doc string that is wrong?

I think there were many places like this where I made an attribute private since it internally changed type from a string to something else and a property would be needed to handle changes to it correctly.

It does not need to be changed I think. Your proposal to make it a property with no setter is a good one.