get and set value for AbstractBeam

[marcus-oscarsson]

Does it make sense to have a get and set value for AbstractBeam or is there no evident value associated with a Beam object

[rhfogh]

I'd say the value of a beam does not really make sense - it would be more obvious to have explicit names for each attribute and/or the info_dict. If it is decided we do have to have one, the least misleading value to choose (Note: it does not seem to be clear what the value should be - that is significant), would be the info_dict.

[IvarsKarpics]

I agree that get_value, set_value makes not so much sense for AbstractBeam. Some time ago we agreed that set, get_value should take or return a single argument and from the class name it is more or less clear what you will get. If we will start to pass a dictionary sooner or later we will mess up things.

[marcus-oscarsson]

I'm in realty not of a really strong opinion regarding this. We implemented the get and set value here for Beam some time ago even before the first discussions on AbstractBeam. This have stayed with us since then maybe partly because it worked well with the concept of set and get_value of the HardwareObject.

I'm not sure that I agree that there is no "reasonable" value for a beam object (in our context), I would not be surprised if it was an object (obviously not mutable) describing the beam. This object could for our purposes include beam size and shape. I do agree that it makes the setting a bit awkward. Its maybe more because for the later reason that I would be tempted to possibly not have a set and get value on AbstractBeam.

Having something that returns all the "attributes" of the beam and not call it get_value might on the other-hand also be a bit odd and even sometimes feel a bit inconsistent. One could imagine that there is a get_value and not a set_value and that the each attributes are set individually. I realize that this would to some extent violate the way we define value of an object and that some of you might object to such an idea, so I'm just going to mention it for completeness (and maybe to have your thoughts).

[rhfogh]

I can see the use of having a single function getting all the data you need about the beam. I just cannot see why it matters that it should be called get_value. As Ivars said, we have a clear agreement on what get/set_value should mean, and the behaviour you are describing is incompatible with that. What is the downside of having the functions you need but just calling them something else?

As for being "reasonable", we agree that Energy.value was an energy (in keV), and the wavelength was accessed separately. If we start having 'value' as a multi-attribute object, we shall have to remember that beam returns an object, whereas Energy, which might have done the same, does not.

Having a getter for the multi-attribute object you want, but on setter, would at least solve the problem of having to get hold of all the diferent attributes any time you wanted to change some of them, like just the widths. But, again, why not 'get_info_dict', in that case?

[IvarsKarpics]

I think it is easier to to continue this discussion with real examples where and how the AbstractBeam is used. From the qt gui perspective there are separate bricks to control aperture (set elliptical beam), slits (set rectangular beam) and beam focusing (set elliptical micro beam). Then there is a feature to draw a rectangular beam which sets the slits based on the size of rectangle. The resulting beam shape and size is evaluated in the AbstractBeam. But there is no feature where user can select beam size and shape simultaneously.
Do you need this set_value with beam size and shape for workflows?

[marcus-oscarsson]

As I said, I don't really have strong opinion. For me, we can go for a set and get for each individual attribute.

I can see the use of having a single function getting all the data you need about the beam. I just cannot see why it matters that it should be called get_value. As Ivars said, we have a clear agreement on what get/set_value should mean, and the behaviour you are describing is incompatible with that. What is the downside of having the functions you need but just calling them something else?

It would be to keep things as similar as possible and try to keep the things we need to remember at a minimum but you are indeed right that it is a bit inconsistent.

As for being "reasonable", we agree that Energy.value was an energy (in keV), and the wavelength was accessed separately. If we start having 'value' as a multi-attribute object, we shall have to remember that beam returns an object, whereas Energy, which might have done the same, does not.

Yes, there are always things we need to remember. Hopefully most of them would be things that in our context makes sense
like energy in Kev. But you do finally have to remember it or check the documentation. I agree that the interface gets much simpler to handle without multi-attribute objects. So I would also agree that setting and getting each attribute would be better

[beteva]

Actually, what we've agreed upon was that the get_value returns what represents the name of the class and not that it returns a single value. That's why for the Energy class we have get_value and get_wavelength. This being said, I do not have strong opinion about what get/set value should be for the Beam class and should there be get/set value at all. In the ideal case there should be a way to ask for a beam size and the underlying software to handle what hardware will be used to achieve it. We also need to know the beam shape (as we draw it), but I am not sure if we want to set the beam shape (this will very much depend on the hardware).
So to conclude:

1. If there is something that can eventually represent the beam get_value and have the same get/set parameters, it is the beam size (horizontal/vertical). Personally I do not care if it is implemented as get_beam_size or get_value.
2. It is handy to have one method, that summarises all the beam parameters, but it can only return a dictionary. It is difficult, though, to have a corresponding set, because we have to either fix the number of arguments (which is a pity) or set **kwargs as an input, which is a pain to handle all the allowed keys.