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.

Sign up for GitHub

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

Jump to bottom

[mount_sample] Simplified mount_sample and related logic - Part 1 #971

Draft

marcus-oscarsson wants to merge 1 commit into develop from mo-queue-mount-sample

Member

marcus-oscarsson commented Jul 12, 2024 •

edited

Loading

Related to issue #970

Removed SampleChanger.load_sample and replaced it with SampleChanger.load

The SampleChanger.mount_sample method uses a method called load_sample that are only defined on a few sample changers and not defined in AbstractSampleChanger. SampleChanger.mount_sample seems to be the only place where load_sample is used. Perhaps one of the Qt developers could confirm that its not used directly by the UI or some site specific qt related code.

Introduced a progress message signal on AbstractSampleChanger
Introduced a centering_method setting that is meant to be application wide on the SampleView HardwareObject


          Simplified mount_sample and related logic

4ee1430

marcus-oscarsson commented

View reviewed changes

mxcubecore/queue_entry/base_queue_entry.py

    
              def mount_sample(view, data_model, centring_done_cb, async_result):

                  view.setText(1, "Loading sample")

              def mount_sample(data_model, centring_done_cb, async_result):

                  HWR.beamline.sample_changer.trigger_progress_message("Loading sample")

Member Author

marcus-oscarsson Jul 12, 2024

view.setText replaced by HWR.beamline.sample_changer.trigger_progress_message

marcus-oscarsson commented

View reviewed changes

mxcubecore/queue_entry/base_queue_entry.py

                   HWR.beamline.sample_view.clear_all()
-                  log = logging.getLogger("queue_exec")
+                  log = logging.getLogger("user_level_log")

Member Author

marcus-oscarsson Jul 12, 2024

user_level_log seemed more appropriate

marcus-oscarsson commented

View reviewed changes

mxcubecore/queue_entry/base_queue_entry.py

-                                  "Mockup",
-                              ):
-                                  return
+                      centring_method = HWR.beamline.sample_view.centring_method

Member Author

marcus-oscarsson Jul 12, 2024 •

edited

Loading

centring_method now on HWR.beamline.sample_view and not view.listView().parent().parent().centring_method

marcus-oscarsson commented

View reviewed changes

mxcubecore/queue_entry/base_queue_entry.py

                       dm = HWR.beamline.diffractometer
-                      if dm is not None:
-                          if hasattr(sample_mount_device, "__TYPE__"):

Member Author

marcus-oscarsson Jul 12, 2024

Removed by adding CENTRING_METHOD.NONE (which is default), seems a bit odd to not center depending on __TYPE__

marcus-oscarsson commented

View reviewed changes

mxcubecore/queue_entry/base_queue_entry.py

                       robot_action_dict["status"] = "SUCCESS"
                   else:
                       robot_action_dict["message"] = "Sample was not loaded"
                       robot_action_dict["status"] = "ERROR"
                   HWR.beamline.lims.store_robot_action(robot_action_dict)
-                  if not sample_mount_device.has_loaded_sample():
-                      # Disables all related collections
-                      view.setOn(False)

Member Author

marcus-oscarsson Jul 12, 2024

This is a bit odd, it will disable the view if sample could not be loaded. Is it really necessary ?

Member Author

marcus-oscarsson commented Jul 12, 2024 •

edited

Loading

This code is used when using the Qt UI and running the queue with the automatic option We need to remove the dependency of the Qt view so that also the web version can use this logic. It seems like there is not alot of sites using this piece of code maybe @rhfogh and/or @MartinSavko could confirm ?

Collaborator

rhfogh commented Jul 12, 2024 •

edited

Loading

The only use for any of these functions in the qt branch is in plate_navigator_widget, functions navigation_item_double_clicked and navigation_table_double_clicked, which both call HWR.beamline.plate_manipulator.load_sample. Not sure whether it might not be better to keep load_sample for plate manipulators (only), rather than calling a function with '_' prefix from outside the class.

Member Author

marcus-oscarsson commented Jul 30, 2024

I agree that SampleChanger._load_sample should not be called from outside of the class but SampleChanger.load could perhaps be used instead so that its more consistent with AbstractSampleChanger. For the code to work without making various type checks the queue load sample mechanism have to be named the same, its less important from a UI perspective and there we could call it load_sample even if it seems a bit unnecessary.

Collaborator

rhfogh commented Aug 1, 2024

From the Qt side there are only two calls, both like
HWR.beamline.plate_manipulator.load_sample(
(table_item.row() + 1, table_item.column() * self.num_drops + 1),
wait=False)

Is it the case that PlateMsnipulator.load_sample 1) is simply renamed to _load_sample, 2) has the same interface?
It would be simple enough to just rename the two calls from the Qt side, but I am not at all at home with plate manipulators, so someone else ought to make the judgement call.

Member Author

marcus-oscarsson commented Sep 10, 2024 •

edited

Loading

@rhfogh The reason for the renaming was that SampleChanger only defines a load method and not a load_sample method. As PlateManipulator is a sample changer and is used as such we expect it to have load method. There was previously a isinstance check but I think that we should move away from that as much (and far :) ) as we can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet