Refactored and added specific actuators list

[rhfogh]

I am not the best qualified person for this. But since the qualified persons are all very busy, I have made free with reorganising and adding changes. Hopefully this will inspire people to tell me where I am wrong and what should be done instead.

The current proposal expands the procedures and actuators to cover the entire UI, not just the beam. It also adds lists of the actuators and attributes that will be covered.

[marcus-oscarsson]

This text is a bit vague, all procedures and actuators are used generally across the interface.

Each file would in my opinion correspond to one namespace, so that one would do for instance:

beamline.run_procedure("QUICK_REALIGN")

or

samplechanger.unmount_current_sample()

One could imagine a namespace for data structures that are shared among the different files, i.e
beamline_types or simply types.

This part of the interface gathers as I originally wrote:

"Functionality that incorporates several different instruments or that does not necessarily belong to one particular 
 instrument." 

[rhfogh]

There are two question, here, I believe.

• One is whether we should use the same pattern, Enums and NamedTuples for procedures, actuators etc. globally across the specification. I believe we definitely should (in so far as possible), and it was not clear from putting the Actuator class inside the Beamline.py file. If it is wider in scope, it should have a separate file.

• Another is how to package up the interface. That AFAIK is still undecided, and several solutions would work. For now I would tend to propose a single, flat, interface. It is not always obvious which commands should go to which component, especialy since some components are by hardware, and some are by process.
  Is set_actuator_value('Omega', 90.0) a Beamline command, a Diffractometer command, a SampleCentring command? Is set_actuator_value('resolution', 2.0) a Beamline command, or a Detector command? How often (if ever) will we have two procedures with the same name that live in diffferent contexts and do different things?

Apart from that, it does not make too much difference if we call samplechanger.unmount_current_sample(), procedures['sample_changer']'unmount_current_sample', or just unmount_current_sample(), as long as we have a clear organisation.

[marcus-oscarsson]

I see,

• My intention is to reuse (when appropriate) the same ActuatorData and ProcedureData throughout the entire API, They were defined here for the moment since it was easier to read in this way (and for now only used here).

• I would say that set_actuator_value('Omega', 90) is either a bemline command or a diffractometer command but not a SampleCentring command (nor a DataCollection command). This since moving omega is not exclusively for sample centering. I would actually want to place it in diffractometer when I think a bit more about it, but I don't really see any reason against having it in beamline.

• set_actuator_value('resolution', 2.0) is my opinion a property of the beamline (or the beamline configuration) not the detector.

• So far we never had concurrent execution of the same type of beamline procedure, they would be limited to things like, quick realing, centre beam, anneal, scintilator ...

• Hm, I would tend to try to suggest some kind of fairly flat namespace that corresponds to the actual beamline layout. The code becomes much clearer to me if we do:

  samplechanger.unmount_current_sample(),

[rhfogh]

Sounds fine - general parts will be commented on later.
Just one minor point:

• resolution is a beamline command.
• detector_distance is (presumably) a detector command.
• resolution and detector_distance are two ways of setting the same thing.

[beteva]

Resolution in definitely a beamline command, as it is a function of different equipment.
detector_distance is only a motor but can very well be a beamline and not detector command.
You can set detector distance without being able to set (calculate) the resolution, but frankly does not make any sense.

[marcus-oscarsson]

I would avoid General here, we dont have to use beamline but the intention with this was to define functions that
are "beamline wide".

[marcus-oscarsson]

Args should in that case be tuple[Any]

[rhfogh]

Hold on: is 'args' the names of the arguments, used to psecify what kind of procedure it is (e.g. 'omega', 'kappa' 'phi')? Or is it that actual values of the arguments for the currently running procedure (e.g. (90.0 127.5, 37.2)), and so undefined when a procedure is not running? I had understood it as the former.

[marcus-oscarsson]

My idea was that its the actual value of the arguments, (e.g. (90.0 127.5, 37.2)),

[marcus-oscarsson]

Not really that depends on the logic that executes the procedure this is just procedure data, however the semantics gets a bit more complicated if we need to consider concurrent execution of the same procedure type. We in that case would ideally (but not necessarily) need a procedure id which is tied to a particular instance of a procedure to handle the different run time scenarios

That aside, we don't really have that need for the moment since this corresponds to procedures involving hardware, like quick realign, we definitely do not want to run several of those at the same time ...

Id even say that its desirable that the logic that executes the procedure prevents several procedures of the same type to run at the same time

[rhfogh]

That is the question, I guess. Do we need a procedure ID separate from the 'understandable' name? I guess the answer is no (for now).

[marcus-oscarsson]

Well the name is needed to identify the kind of procedure and the ID its runtime instances, and I think its enough with the name for the moment. But I might be wrong ?

[marcus-oscarsson]

These are "beamline wide" procedures, procedures specific to a "component" would not be accessed through this method, but through a function specific to that "component".

[rhfogh]

For consistency, would it not be better if all procedures were within a component - and we had a component called 'beamline'? It is a little messy to have two different places or dictionary levels to look for procedures.

[marcus-oscarsson]

I'm not sure what you mean by component but if its like beamline, dffractometer ... then yes

[marcus-oscarsson]

Actually the return data could as well be Dict[str, ProcedureData], where the key is the procedure name

[rhfogh]

Yes it should. I forgot to updat that line.

[marcus-oscarsson]

Why don't we simply add a signal that emits procedure name, progress value and optional progress message with a handler that looks something like this:

procedure_progress_handler(procedure_name:str, value: Any, message:float):

[rhfogh]

Good idea.

[marcus-oscarsson]

I would remove Generic

[marcus-oscarsson]

This goes a bit too far in my opinion, we should not allow any attribute to be modified through this function. Only a well defined subset which remains to be defined ...

[rhfogh]

Yes, it should be a well-defined subset (see below for a proposal). But as you will see I think it would work both for Kappa, resolution, beam_shape, beamstop, and wavelength, ... and that we should use the same mechanism even on fixed-energy beamlines (just setting the state to 'FIXED'). Then I can get hole of the wavelength in the same way without first having to check if the beamline is tunable.

[marcus-oscarsson]

Okey, I see, Great !

[marcus-oscarsson]

That is one way to see it, although I would not necessarily view it as some kind of archetype (if that is what you mean ?) but something that carries data to the specific actuator.

[marcus-oscarsson]

I don't think that its actually needed, I can imagine that it have been or perhaps are used in cases where it takes a long time between an "inital" move and "actual" move. We are not using this in our UIs as far as I know.

[marcus-oscarsson]

Of course, at least in one direction ...

[marcus-oscarsson]

So these are actuator names ?

[rhfogh]

That was the proposal - in order to get a single, high-level set of names that did not depend on local variations. I am fully prepared for people to knock that one down (though I did thake the Alignment and Centring from Martin Savko), but I thought it would be useful to start a discussion at least.

[marcus-oscarsson]

I think its great !

[marcus-oscarsson]

Lights usually have a in/out property as well as intensity, you can see it a bit as on, off and intensity. They are moved in and out to not interfere with the beam or other equipment.

[rhfogh]

The On/Off is handed as a separate property, below.

[marcus-oscarsson]

Ahh, ok

[marcus-oscarsson]

So I would say that aperture and slits are a kind of beam definers, these are only interesting if we need to somehow fine tune i.e the aperture or the slits from the UI. I dont think we have that need at the moment ...

[rhfogh]

I agree. We should probably agree on the names, for harmonization purposes, but otehrwise elave them for each beamline to use or not as they see fit.

[marcus-oscarsson]

Okey, yeah thats fine by me

[marcus-oscarsson]

I guess zoom could theoretically be a float

[marcus-oscarsson]

So they are not actuators, but values ...

[rhfogh]

Yes. I know it is a bit of a stretch. But 1) It would be nice to use the same code to get e.g. the wavelength whether the beamline is tunable or not. 2) From a UI point of view e.g. photon_flux is a continuously updated value that is shown, that sends value_changed signals, that can be down (UNUSABLE), or working. It just cannot be modified from the UI.

[beteva]

I believe there is a confusion what an ´actuator´ should act like. From what I see, you are describing a ´mover´. Actuator for me should be something that has much limited set of states - IN(ON), OUT(OFF) and UNKNOWN.

[marcus-oscarsson]

Dear all,

Thanks for the really nice input, there are several good suggestions and refinements here.

It seems like you guys are really fast at answering, I just wanted to comment on a few things before merging so that we could continue the discussion in an iusse.

So I will finally wait a little bit before merging so that we can settle a few things while we are at it and then Ill merge ...

[rhfogh]

Antonia: 'Actuator' may be the wrong name, then.

[rhfogh]

Marcus: I think I should probably get some imoprovements in before the merge. We just need to see what we agree should be done quickly. How do we go about settling that?

[marcus-oscarsson]

Okey

Thanks again for a good initial discussion, we are one step closer to have something well defined now :)

So it seems to me like we have more or less similar ideas, it looks like we need to continue to discuss the scope of the beamline actuators and then the name space division (if any).

[marcus-oscarsson]

@beteva We have in our suggestion generalized Actuator to also include continuously moving motors. It unifies actuator and "movable" (that we have today)

[marcus-oscarsson]

@rhfogh hm, how about this, you can go trough the comments and change those things that are immediately obvious what to do with (if any) and keep the rest for further discussion ?

[rhfogh]

I am going through now, dividing into agreed / leave for discussion / clarification preferred. I'll put that up, and then make a modified PR based on the answer.

Back soon.

[marcus-oscarsson]

Perfect, good idea ! (no rush)

[rhfogh]

This is what I have come up with. I shall make a start, locally tonight, and expect to make a new PR tomorrow, after feedback on this. I shall also send out a meeting reminder and draft agenda.

Agreed (for now):

• Procedure and Actuator code to be defined at the top level and used across the UI.

• GenericActuatorData, ActuatorState, TwoStateValue, and the actuator functions and signal handlers are roughly right.

• ProcedureData and related functions and signal handleers need clarification (see below)

• procedure_value_changed_handler(ProcedureData): should be replaced with
  procedure_progress_handler(procedure_name:str, value: Any, message:float):

• MOVESTARTED to be removed.

• The list of specific actuators and their types are roughly right.

• We should split the current beamline.rst file into a beamline part and a generic part.

Immediate Clarification:

• ProcedureData, args and kwargs. Should args and kwargs be the names of the parameters, with their default values, or should they be the values instead?

  I would vote for the names.

  1. it seems to be useful configuration information, so you can see not only which procedures are supported but also what their interfaces are, locally.
  2. It allows individual beamlines to add specific parameters - with defaults - without breaking the interface.
  3. I do not see where the actual values are needed. ProcedureData are not used in run_procedure.
     They are used in get_procedures (where no actual parameters are available), and in procedure_state_changed_handler, and that is all. What is the use case?

Leave for discussion:

• Rename of 'GenericActuatorData'?

• The name of the components, and the division of actuators (and later procedures and other functions) between the components. Can I propose that Marcus does a draft proposal, after the merge?
  (Components, as I understand it, include Beamline, Difractometer, but also SampleCentring, Queue, Login, ...
  I could not come up with an exhaustive and non-overlapping classification ofactuators or procedures, so someone else had better make a start)

• 'Immovable actuators' probably need some discussion, but we can leave them in the proposal

• For now we use 'name' (e.g. 'QuickAlign') as identifier for procedures. It follows that:

  • We must have names unique globally (not just within a component)
  • We do not want a cmd_id for now, but can introduce one later if necessary
  • The code must ensure that we can never run two instances of the same procedure in parallel.

[rhfogh]

Thinking back, it seems to me that the top-level functions get_actuators(), get_actuator(name), and set_actuator_value(name, value) are all we need, and adding functions like diffractometer.set_Kappa(value) does not make much sense. But we can discuss that. Meanwhile I am waiting for clarification (and ideas on how to handle the PR rejection while keeping the discussion) before I do a new PR.

[marcus-oscarsson]

@rhfogh Just make the changes you need to, then push (again) and this PR will be updated automatically with your new changes ...

[marcus-oscarsson]

Thanks for your nice summary, It seems like we agree on most things

A small clarification, that might help to shed light on the differences:

In general I think that we have similar ideas there are maybe slight difference in paradigm, the outcome seems to be roughly the same but there maybe some important differences there. I've tried to keep my original proposal functional with data structures that are used to pass information. Data structures that for instance describes an actuator or a command and that are easily serializable to (and from) text based formats such as JSON. While it seems like you have more of an object oriented approach ?

• GenericActuatorData is a structure that holds the data that describes the "state" of an actuator. There is nothing really generic with it from my perspective so it would be better to simply call it ActuatorData. I can as you mention once this is merged, make a name space and naming proposal.

This would also avoid the confusion for people adopting the interface to think that they should create a specific ActuatorData object for certain actuators.

• In this light the ProcedureData args and kwargs simply contains the values needed for procedure execution. I think there is a mistake in the (sorry for that, it might cause some confusion)

  run_procedure(name:str, *args, **kwargs)

It should simply be

run_procedure(data:ProcedureData)

and

get_procedure(name:str) -> ProcedureData

returns the ProcedureData with the default values

We could alternatively if we think that we in the future need the parameter names, also add something like arg_names and kwarg_names ?. The need have to day, and the way its used, is to pass the actual values.

I agree that we should avoid having for instance,

diffractometer.set_kappa(...)

Cheers,
Marcus

[rhfogh]

@marcus-oscarsson
ActuatorData is generic from a typing point of view. value, upper_limit, lower_limit, and allowedValues must be of the same type (or list thereof), and the type can be either float, string, pair-of-floats, ... If we want to formalise that we need some kind of genricity, but OK, it is probably not worth the hassle.
Rather than jumping through hoops, I shall just remove the typing on the relevant fields, and put in a couple of comments.

[rhfogh]

I am not sure I like run_procedure(data:ProcedureData)
If we do it that way, we would have to write, e.g.,

run_procedure(ProcedureData('store_image_in_lims', None, (frame_number,), {'motor_position_id':47}, None, None, []))

The other way we could just do

run_procedure('store_image_in_lims', frame_number, motor_position_id=47)

It does not matter so much when we are sending information from the HO level - all the info needs to travel one way or the other, and we might as well have a single function that sends a dictionary than a bunch of individual functions. But when we are sending information to the HO level it becomes irritating to pass dummy values for four parameters that will never be needed.

If you compare, ActuatorData is pure config information except for value and state, and we use set_actuator_value to pass information the other way.

Maybe we need to talk a bit more about how we want to use get_procedure and get_procedures?
I gather that the intention is that the beamline can present procedures (or their variants) that are unknown to the UI?
tool_tip and display_name are clearly config information to the UI, name is an identifier, and state is the only piece of information that definitely needs to be passed back to the UI. But what are the args and kwargs needed for, when passed to the UI? I had assumed they were needed for config, and to publish what functions the beamline supported. If not, maybe we do not need them at all?

[marcus-oscarsson]

Hm, I see your point but:

We have to remember that this is an API for the UI and secondly the procedures we are talking about are a limited set of procedures that involve a set of instruments to perform for instance alignment, aneal and similar tasks. Procedures that exists on many beamlines, that are "beamline wide" but that are done differently depending on the beamline and available equipment. The idea is to provide a mechanism where its easy to add such procedures to the UI. The number of these procedures could vary from beamline to beamline some have many and some none.

store_image_in_lims would not be such a procedure, it would belong to an entity called lims and are the same regardless of beamline. Well we at least assume that all beamlines have for instance store_image_in_lims and mount_sample, what is done behind the scenes are a different matter, Actually store_image_in_lims would never even be called by the UI.

So since we trigger these things from the UI one would rarely perform:

run_procedure(ProcedureData('store_image_in_lims', None, (frame_number,), {'motor_position_id':47}, None, None, []))

What one would do is to execute:

get_procedures()

That returns a list of ProcedureData with default values, that can be displayed in an appropriate manner in the UI. The user then chooses to execute one of those procedures, the client then sends the ProcedureData over to the server. The args and kwargs of PorcedureData might have been modified in the UI before sent. The backend then simply exeutes:

run_procedure(ProcedureData)  # Where procedure data is the data sent from the client

Actually I see little need for explicitly running,

run_procedure(ProcedureData('store_image_in_lims', None, (frame_number,), {'motor_position_id':47}, None, None, []))

I hope this clarifies my ideas a bit ?

Marcus

[marcus-oscarsson]

For the typing information, I think its good idea to specify the types of value, upper_limit, lower_limit, and allowedValues. My thought was simply that GenericProcedureData implied that something more might be needed, which might be confusing when one tries to adopt the API.

[rhfogh]

@marcus-oscarsson
It does clarify your ideas, but I still have open questions:

I picked store_image_in_lims as a pre-existing function that had both positional and keyword arguments, so it does not have any particular significance.

On the appropriate parameters for run_procedure, either approach would work. It sounds like we agree that args and kwargs (if we add that one) are the only parameters actually needed, but we can opt for whichever system people find easiest to deal with. I'd put that one up for discussion.

More important (maybe), are we going to use the ProcedureData approach only for beamline-specific procedures? Using it more widely would maybe simplify things, and allow for beamlines writing variants of standard procedures with extra parameters, whereas fixed standard procedures would be harder to vary. But again, we can leave that one for discussion.

My biggest question is how you are going to display a new prodecure 'in an appropriate manner in the UI'. Suppose you did not already know exactly how the function worked and had ProcedureData like this:

name: 'reset_data_collection'
args: (90.0, 'B', 5, None)
kwargs: {'force':False}
...

How are you going to decide the appropriate way to display that? Without a type field it will be hard to set up an appropriate widget (though I guess you could pass all parameters as strings). Without at least a parameter name it would be impossible for the user to know what the parameters meant (unless you put the documentation in the tooltip). If you really wanted a neat result you would need both a default value and at a minimum a widget-type string to guide the UI generation. Maybe the best bet (for now) would be to get rid of args and have only kwargs, which would give you a parameter name and a (default) value for all parameters.

[marcus-oscarsson]

Great, okey I see.

My idea with the beamline Procedure is to provide an extendable way to run procedures that are beamline specific or common between beamlines but often performed in different ways. In this sense they are both for beamline specific and for standard procedures. For example "center beam" which might be done differently across beamlines but exists on most.

We might need to add as we have mentioned before the argument names if we like to provide a default procedure view, which might be a good idea. Similarly to how we handle workflows today. My original idea was that the UI would have predefined views for the set of Procedures it handles. Which is displayed when the user triggers the execute Procedure action in the UI. This is for instance how its done in MXCuBE3 today.

Cheers !
Marcus

[rhfogh]

@marcus-oscarsson
In that case I think I shall get rid of args, put kwargs as an OrderedDict, and have run_procedure(name, **kwargs). That gives both variable names and values/defaults, and seems to be a nice, simple starting point (and it is my proposal, after all). We can then discuss starting from there. I shall push my changes and send out a reminder email shortly.

[marcus-oscarsson]

Hm, okey but then you don't know which parameters that are optional or not, but that's okey for now I guess. Its a good starting point. Is by the way OrdredDict really needed ?

[marcus-oscarsson]

Looking at your latest changes;

The only things that should, in my opinion, be in general.rst are the data structures the functions should remain in beamline. The data structures can be reused by other parts of the API but the functions still belong to beamline

[rhfogh]

A plain dict might suffice, but ordering it does not cost much, and if we really want to display this without handcoding the interface, it might be useful to be able to order the parameters in order of importance.

It would be preferable to be able to call common functions without knowing the beamline, which would require that mandatory parameters were always the same, and that any additional parameters were optional. Whether this is fully realistic is of course another question.

[marcus-oscarsson]

The functions should maybe still be in beamline ?

[rhfogh]

@marcus-oscarsson
Whether e.g. set_actuator_value belongs in beamline or in 'general' I think goes with the logical division into components, which I hope you will be doing a proposal for. I am not clear if 'beamline' is for 'everything', 'everything that does not belong to a specific piece of apparatus', or 'a specific set of functionality'. And since set_actuator_value potentially is used for both detector_distance, beam_shape, and Omega I thought it needed some kind of central position.

[marcus-oscarsson]

Okey, thanks for all the input so far, I think its really starting evolve into something nice.

I hope to make one PR with a diffractometer api and another one for the "component"/namespace division next week.

Marcus

[rhfogh]

Excellent, I look forward to seeing that. I shall put in some comments for SampleChanger, and maybe try to look at queue (unless you have a suggestion for a better place to start).

[marcus-oscarsson]

Okey great !, Ill be away until Tuseday so I wont be able to respond to any comments until then. Sure you can get started with the queue, another thing that could be interesting for you guys is the workflow part of the API.