[WIP] AbstractDiffractometer

[beteva]

AbstractDiffractometer class:

• inherits from HardwareObject and uses wait_ready, abort methods and the state handling.
• defines some methods, properties and the DiffractometerHead, DiffractometerPhase enums.
• defines some scan types
• in the init method defines standard access to different equipment (motors, actuators and complex/compound equipment). The access relies on the roles, which are defined in the configuration file. To be discussed if checks are to be done. This is a typical xml configuration file:

<object class = "MicroDiffractometer"
  <username>MD2S</username>
  <exporter_address>id29md2:9001</exporter_address>
  <motors>
    <object role="omega" hwrid="/udiff_omega"/>
    <object role="phiy" href="/udiff_phiy"/>
    <object role="phiz" href="/udiff_phiz"/>
    <object role="sampx" href="/udiff_sampx"/>
    <object role="sampy" href="/udiff_sampy"/>
    <object role="focus" href="/udiff_phix"/>
    <object role="zoom" href="/udiff_zoom_as_motor"/>
    <object role="front_light" href="/udiff_frontlight_intensity"/>
    <object role="back_light" href="/udiff_backlight_intensity"/>
    <!-- <object role="horizontal_alignment" hwrid="/udiff_phiy"/>
    <object role="vertical_alignment" hwrid="/udiff_phiz"/>
    <object role="horizontal_centring" hwrid="/udiff_sampx"/>
    <object role="vertical_centring" hwrid="/udiff_sampy"/> -->
  </motors>
  <nstate_equipment>
    <object role="fast_shutter" href="/udiff_fastshut"/>
    <object role="scintillator" href="/udiff_scint"/>
    <object role="diode" href="/udiff_diode"/>
    <object role="fluo_detector" href="/udiff_fluodet"/>
    <object role="cryostream" href="/udiff_cryostream"/>
    <object role="front_light" href="/udiff_frontlight_inout"/>
    <object role="back_light" href="/udiff_backlight_inout"/>
    <object role="capillary" href="/udiff_capillary"/>
    <object role="aperture" href="/udiff_aperture"/>
    <object role="zoom" href="/udiff_zoom"/>
  </nstate_equipment>
</object>

MD2 implementation - overload or fill in the abstract methods.

[rhfogh]

Can I recommend you return a copy? It takes little longer, and then the internal dictionary is not exposed.

[beteva]

Why not indeed. I guess that this method will be quite often overloaded in the concrete implementation.

[rhfogh]

Not sure I understand this one.

Small stuff: the 'method' parameter is actually transferring a value not a method.

Bigger stuff, 'update_value' is the same name as for Actuators, but the calling interface is different. Since the diffractometer is not an actuator this is not fatal, but it could be confusing.

Also, imaging value is None and 'method' is , say, UNKNOWN. This function will set curr_value to UNKNOWN and emit valueChanged. 1) curr_value is not persisted (so what effect does it have?), and could be either a Phase or a Constraint. 2) How much sense does it make to talk about the 'value' of a Diffractometer'? 3) What kind of function would take this signal and be prepared to receive either a Phase or a Constraint? 4) What hapens when someone at a beamline use htese generic-named functions for yet a third parameter?

I cannot make suggestions, since I d not understand what this is for, but could one make separate update_phase and update_constraint, and deal with values changing by another way, e.g. the equivalent of self._nominal_phase etc.?

[beteva]

Hi @rhfogh, you are right, maybe _update_value is more appropriate, as it is meant for internal use only.
The idea behind is that there is no need to define specific update value for each method, that needs it, but one generic, that can take as a parameter (the method) which is the method or the property when you want to get a recent value. This is probably a bit personal style - I've implemented update_phase and update_constraint at first and than changed.
As you correctly pointed out, there is no meaning for a value of a Diffractometer, but still valueCjanged can be emitted.

Also, imaging value is None and 'method' is , say, UNKNOWN. This function will set curr_value to UNKNOWN and emit valueChanged.

This is correct. But as we use it only internally, is also a way to be sure that the one, using the method knows what is going on.

deal with values changing by another way, e.g. the equivalent of self._nominal_phase etc.?
I do not think that the phase (or the constraint) should have a persistent value.

[rhfogh]

What you say makes sense, but there are still things I wonder about.

1. What you are passing is not a method, but the value you get from calling it. method=self.get_phase(), not method=self.get_phase. Why name the function parameter 'method'?
2. The only thing that function does that has any effect is
   self.emit("valueChanged", (value,))
   Why not code that directly instead of calling this function?
3. What is supposed to catch that signal (that may have either a Phase or a Constraint as parameter)? What does it do ?
4. Does it make sense that the AbstractDiffractometer emits valueChanged and limitsChanged (as it says in the documentation) when diffractometers do not have a value? Would it not make sense to give it a name that better reflects what it is doing?

[beteva]

1. What you are passing is not a method, but the value you get from calling it. 
      method=self.get_phase(),  not method=self.get_phase.  Why name the function
      parameter 'method'?

You are right, I've changed the name of the method and its parameter.

2. The only thing that function does that has any effect is
   self.emit("valueChanged", (value,))
   Why not code that directly instead of calling this function?

Just for not repeating the code (at least twice).

3. What is supposed to catch that signal (that may have either a Phase or a Constraint
     as parameter)? What does it do ?

The GUI that wants to update the label, when the diffractometer changes the Phase ...

4. Does it make sense that the AbstractDiffractometer emits valueChanged and
    limitsChanged (as it says in the documentation) when diffractometers do not have a value?
    Would it not make sense to give it a name that better reflects what it is doing?

This makes the code more versatile in respect of the GUI.

[rhfogh]

This looks good, but maybe it is incomplete? I see no mention of vertical_centring etc. in the AbstractDiffractometer code, but I do see mention of phiy, phix, sampx etc. Should this not be homologised?

It would be great if there was a way of putting in the code which role names were mandatory and how they should be spelled, but if we cannot find a good way we shall of course have to do without. Presumably we cannot make the motors mandatory, in case some beamlines lack then? Could we require that roles should eb set (in the dictionary) in the init so that you can only add motors in the config if they are already in the init?

Would it be useful with more detaild docs for some of the centring motors? If we have a vertical instead of horizontal omega axis, what might taht do to the identity of horizontal_centring, vertical_centring etc.?

[beteva]

@rhfogh I was planning to find a schema and put it in the documentation with all the pre-defined roles. Everybody's input is welcome. @ALL, if you have such a scheme, it will be great if you can share it.

It would be great if there was a way of putting in the code which role names were mandatory.

That would depend on the diffractometer and its head.
What we could do is to decide for a list of mandatory roles, which we check in the init and if not specified in the config, we set the object as None in the dictionary of the motors. This is a bit too much in my opinion, but the only was to make some check against the correct spelling.

Could we require that roles should be set (in the dictionary) in the init so that you can only add motors in the config if they are already in the init?

I would avoid limiting the number of roles for either motors or nstate equipment. This make the AbstractDiffractometer less versatile, especially taking into account the new head types, used by the SSX experiments.

[rhfogh]

I had in mind something like the way it works for yaml objects. There properties can only be put in the configuration if the corresponding attributes are defined in the __init__ (if hasattr(object, 'newrolename') returns True, basically). BUT there is nothing to stop you from adding additional role names in the __init__ of subclasses and then putting the relevant data in the config files, which will give you as many additional (sub)roles as you want. The point is that doing something similar here would make it visible in the code which roles there are for a given object, and not allow people to code to roles that only appear in config files. Ideally these should be attributes, not just dictionary members, so that linters will pick them up. But hey, we cannot be ideal all the time.

[meghdadyazdi]

Why def get_motors(self) is needed here? and why it returns list?
It is defined in Abstract and it it returns dict there.

[beteva]

Hi @meghdadyazdi, well spotted. Somehow, when committing I've included the wrong implementation of the get_motors.

The method is overloaded in MicroDiffractometer, as we want only the currently available motors to be used. The dictionary of these motors is not only static - depending on the head (plate, kappa, ssx ...) but also dynamic - depending on the constraints applied (as in the latest implementation of the MD java software)