Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sample changer - discussion #4

Open
rhfogh opened this issue Apr 30, 2018 · 14 comments
Open

sample changer - discussion #4

rhfogh opened this issue Apr 30, 2018 · 14 comments

Comments

@rhfogh
Copy link
Contributor

rhfogh commented Apr 30, 2018

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

  • mount(location)

  • unmount() # MO has unmount(location) - is the location mandatory/optional/unnecessary?

  • get_state()

  • get_sample_list() # MS calls it 'get_present_samples'

  • get_loaded_sample() # **NB MS does not have this, but surely this functionality is necessary? **

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?

  • get_error_message() # MS

  • prepare_sample_loading() # MS

  • select_location(location) # MO

  • scan_location(location) # MO

  • get_full_state() # MO

@marcus-oscarsson
Copy link
Member

marcus-oscarsson commented May 3, 2018

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 :)

  • The idea with the sample changer commands is exactly that it is infinitely extendable, since different sample changers might have different capabilities, some have grippers some does not, some needs to be "de-freezed" others not and so on.

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 ?

  • For us prepare_sample_loading is more a beamline global activity than a sample changer one so I've
    added this method to the beamline API (see my latest PR Beamline API #5 ). But it can as well go under the sample changer category.

  • We can also add a get_error_message function if one feels that retrieving the full state through get_full_state is inappropriate for certain contexts.

  • The location for unmount_sample is not mandatory, the sample is simply unmounted to where it
    was loaded from if nothing is passed (see unmount_sample documentation #6)

Cheers,
Marcus

@rhfogh
Copy link
Contributor Author

rhfogh commented May 3, 2018

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
Copy link
Member

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
Copy link
Contributor Author

rhfogh commented May 3, 2018

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
Copy link
Contributor Author

rhfogh commented May 18, 2018

I have several questions about the SampleChanger API:

  • Is a SampleTuple not the same as a SampleNode without children? Could/should SampleTuple and SampleNode not be merged into one?

  • Is the state field in both not a SampleChanger state, rather than a Sample state. And if so should it not be removed? Or is there such a thing as a Sample state?

  • What is the difference in use betweem mount and select? My guess is:

    - Plates, Pins, and MultiPins can be mounted, whereas Baskets or Pucks cannot.
    
    - Mounting a Pin automatically selects the one crystal (location) it contains.
    
    - For a Plate or MultiPin you must select a location within the mounted object. 
    

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
Copy link
Member

Hi thanks for taking the time to look at this:

  • Yes they can definitely be merged, the difference is that SampleNode is not always a Sample but might be something that contains samples. As you say children can simply be left NULL/None.

  • Actually yes, as its often used its more as a Sample Changer state than a sample state, so for Samples it would be basically MOUNTED or UNMOUNTED, but one could perhaps imagine another state. I would actually be tempted to remove this state at least for any state that can be derived from the sample changer. The reason for that it exists like this today is that its fairly easy to update just one node in a nested structure if its fairly self contained.

  • Selecting a basket or cell would simply move the sample changer to that location, while mounting a sample would put a sample on the goniometer. I think selecting a sample was possible for instance in the SC3 (maybe @beteva can confirm that). Its not an operation that is actually used very often from the data collection UI, its more for maintenance when you would like to access a sample manually. Today the location is limited to "loop resolution", that is: we cant pinpoint a crystal with a location but one could easily imagine such a location.

  • I'm not entirely certain without actually taking a look on how selected and mounted is handled, it might even be sample changer specific. But one could make the reasonable simplification to say that if a sample "S1" is mounted no other sample "Sn" can be be selected.

@rhfogh
Copy link
Contributor Author

rhfogh commented May 22, 2018

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
Copy link
Member

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

@rhfogh
Copy link
Contributor Author

rhfogh commented May 22, 2018

@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
Copy link
Member

  • Selecting would be the same as move to, nothing is put onto the goniometer. It simply moves the internal mechanics of the sample changer to a position that corresponds to "location". What that means is different from sample changer to sample changer.

  • Mounting actually puts something on the gonio

  • Selecting in this way have nothing to do with selecting a crystal or location within the
    mounted object, that is another concept.

@rhfogh
Copy link
Contributor Author

rhfogh commented May 22, 2018

@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
Copy link
Member

marcus-oscarsson commented May 22, 2018

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
Copy link
Contributor Author

rhfogh commented May 22, 2018

@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:

  • mount: Mount a pin.
  • select: select a container on the samplechanger
  • multipins and plates are TBD, as far as the interface is concerned

@marcus-oscarsson
Copy link
Member

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 ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants