mxcube / ui-api

Repository for specifying the API used between the MXCuBE user interface and its backend
GNU Lesser General Public License v3.0
0 stars 3 forks source link

sample changer - discussion #4

Open rhfogh opened 6 years ago

rhfogh commented 6 years ago

We now have the sample changer proposal from Marcus. and the sample changer (among other things) from Martin. It would be nice to get the discussion started. I would summarise agreements and questions as follows (questions in bold):

Minimum requirement

Command handling

MO has get_available_commands, get_command_state, execute_command. This has the advantage of being infinitely extendable, but the problem that no functionality can be assumed to be present (or consistently named) across sites. Is there a need (and possibility) for some commands (e.g. abort) to be universally supported?

The commands proposed here, between MS and MO, are: on, off, abort, park, calibrate, dry, home, defreeze, reset_sample_number, change_gripper

Procedure dependent?

The MO and MS lists have commands that do not match and that may depend on what kind of procedures are envisaged - as a non-crystallographer I cannot have an opinion. How do these fit into a universal UI?

marcus-oscarsson commented 6 years ago

Hi all,

I'm happy to have some discussions around these things, please feel free to update the first draft that we have added with additional PRs or why not continue the discussion here :)

I think that we can definitely imagine a set of "standard" commands, for instance on, off, abort. But it would perhaps be a bit clumsy to enforce those commands for the cases when they don't apply ?

Cheers, Marcus

rhfogh commented 6 years ago

Hi Marcus,

Sounds good, what you are saying.

My main concern is to make the interface as shared and uniform as possible, so you can code as much as possible in a beamline-independent way. That means that anything that has a meaningful interpretation across beamlines ought to have a globally defined command or keyword. E.g. the command to move the detector should not change, depending on whether the local hardware object is called 'detector_dist' or 'dtox'. I would say it was OK if some functions are no_op at some beamlines, e.g. if prepare_sample_loading is globally defined even though in some places it does not do anything. Obviously commands to change gripper or rotate the kappa motor should throw an error in places where the operation is actually impossible (probably NotImplementedError), but I would say that anything that is meaningful across multiple beamlines, like rotating kappa, should have a globally agreed name.

Does that make sense?

Rasmus

marcus-oscarsson commented 6 years ago

Hi Rasmus,

I makes alot of sense :), I think its a good idea to come up with a list of commands that are common across different beamlines. I also think, like you, that such a command if invoked even though its not implemented should raise for instance NotImplementedError.

I think one of the purposes with this API is to define that common "vocabulary" like move_detector which most likely in our case would be something like set_resolution, since we are working on the API between the UI and the backend.

So what you are missing is a more detailed list of sample changer commands that are more or less common between the different sample changers ?

Cheers, Marcus

rhfogh commented 6 years ago

Hi Marcus,

Yes, I think that while a keyword-based interface like execute_command is a very good idea, for the simplicity and flexibility, it should be coupled with a list of minimum/standard commands with their associated semantics, to minimise divergence. Exactly what should be in that list will be worked out as we go along, the main thing is that we try to get it made.

Yours,

Rasmus

rhfogh commented 6 years ago

I have several questions about the SampleChanger API:

If this is correct, how do you distinguish between the currently selected and the currently mounted sample? What happens if you give a location to mount/select that does not match with the rules? If this is not correct, how does it work, then?

I have made a PR to harmonize the handling of commands / procedures in SampleChanger and Beamline, but the functions commented on here are not affected.

marcus-oscarsson commented 6 years ago

Hi thanks for taking the time to look at this:

rhfogh commented 6 years ago

I commented on the merged pull request, so in case that comment is hard to find:

You are asking why we need the command_category field in the ProcedureData - what the UI can use it for.

I got it as the Command-Category from sample_changer.get_available_commands():

def get_available_commands() -> List[List]: """ Retrieves a List (of Lists) of sample changer specific commands, The List has the following structure:

["Command-Category_1", [ ["cmd_1", "DisplayName", "Comment/Help-text"], ... ["cmd_n-1", "DisplayName", "Comment/Help-text"], ["cmd_n", "DisplayName", "Comment/Help-text"], ] ], ...

I assumed that it was needed for setting up the UI, classifying and displaying the commands by type, and thought it was better to have the category as a data element than as an organising principle. If it is not needed we should certainly get rid of it.

marcus-oscarsson commented 6 years ago

I see, yes that is better ... thanks !

rhfogh commented 6 years ago

@marcus-oscarsson

On selecting / mounting, I would be happy if someone could explain the interplay between selecting/mounting commands; pins, multipins, and trays; and the various containers. Physically there seems to be two different operations: 'mounting' a sample holder on the goniometer, and 'moving to' a particular location or sublocation to look at within it. Standard loops have only a single (sub)location so here the two operations go together, but in other cases they do not. The 'moving to' operation could be termed 'selecting', but 'selecting' could also simply mean 'set as current' with no associated physical movement. Similarly 'mounting' could be used as 'moving to' when you are moving from one well to another on a mounted plate.

How are these things actually defined, at the moment?

marcus-oscarsson commented 6 years ago
rhfogh commented 6 years ago

@marcus-oscarsson Thanks, that does clarify things. But how do people select a specific location and crystal on a plate, then, if it is using neither mount, nor select?

marcus-oscarsson commented 6 years ago

Hm, well plate experiments are still not very common so I'm not so experienced on all the different scenarios and possibilities when it comes to those. However selection within a plate is done through 2d-centring, we could simply imagine a location with a plate coordinate part. Its not very complicated from a software point of view, its simply not used because of limitations in the hardware. Like re-positioning a grid accurately.

I think they are also using grid like or other ways of "automatic" collection when doing these experiments so there is less need for selecting a specific coordinate when mounting. Although, the exact needs in all scenarios should be further investigated (not pressing for us at the moment)

rhfogh commented 6 years ago

@marcus-oscarsson This is being discussed in ISPyB, where people are talking about multipins, plates etc. There they are distinguishing quite clearly between selecting a drop ('BeamlineSample'), and selecting a crystal or location within the drop ('BeamlineSubSample'). I think that will have to be clarified at some point.

But for now I guess the summary is:

marcus-oscarsson commented 6 years ago

I see, I think its already quite clear but its great if we can clarify it further. As for ISPyB their needs are maybe more towards data modelling and thus have different needs.

The problem usually becomes the same regardless of plates/multipins/streams ... you cant mount something if you don't know where it is ...