[feat][METEOR-996] Add TFS3 posture manager for multi-posture imaging #3005

patrickcleeve2 · 2025-01-23T07:35:03Z

Adds:

TFS3 posture manager for multi-posture imaging functionality.
SampleStage virtual component

K4rishma · 2025-01-31T17:09:50Z

Should not we merge TFS2 with TFS3 posture manager and convert into a single posture manager? TFS3 is advanced version TFS2. With this logic, we will not need TFS2.

src/odemis/acq/test/move_test.py

K4rishma · 2025-01-31T17:33:21Z

src/odemis/util/transform.py

@@ -1298,3 +1298,17 @@ def __init__(
    @staticmethod
    def _estimate_matrix(x: numpy.ndarray, y: numpy.ndarray) -> numpy.ndarray:
        return _optimal_rotation(x, y)
+
+def _get_transforms(r: float) -> Tuple[numpy.ndarray, numpy.ndarray]:


As it is only for rotation and we are dealing with different transformations, maybe we can make it clear in the function call? For e.g. _get_3drotation_tranforms. Although I am not sure if adding a number to the function name is a good idea.

src/odemis/acq/move.py

K4rishma · 2025-01-31T17:51:27Z

src/odemis/acq/move.py

+        elif posture == FIB_IMAGING:
+            return stage_md[model.MD_FAV_FIB_POS_ACTIVE]
+        elif posture == MILLING:
+            md = stage_md[model.MD_FAV_MILL_POS_ACTIVE]


Three possible situations to keep the favorite positions in sync with other favorite positons

Maybe nice to specify in _metadata.py that the milling favourite position is not calculated at regular stage bare tilt and rotation, etc ... It gets clarified in the function call that it is different from other favorite angle positons, or

Keep the regular tilt, rotation in the favorite metadata and do the conversions later?, or

Another special metadata to include this unique metadata

Good point, I've added an additional explanation to the _metadata.py to explain the difference between it and the others. I think it is best to keep it defined as this, as it's waht the user expects to see (and the number they care about), as opposed to stage titl

src/odemis/acq/move.py

src/odemis/acq/test/move_test.py

K4rishma · 2025-01-31T18:16:05Z

src/odemis/acq/move.py

+        :param pos: (dict str->float) the initial stage position.
+        :return: (dict str->float) the transformed position.
+        """
+        # NOTE: this transform now always rotates around the z axis (180deg)


In TFS3, should not the logic of transformation from SEM to Meteor and back used from TFS2? If we do not use it, then we cannot enforce imaging at a constant FM imaging plane.

or

Do the 3d coordinate transformations take care of it such the features saved in FM are robust?

Combined with the raw-coordinates, saving the full position of the features (XYZRT) is enough to return to the same z-plane. See other comment about need the feature refactor too

patrickcleeve2 · 2025-02-03T07:53:51Z

Should not we merge TFS2 with TFS3 posture manager and convert into a single posture manager? TFS3 is advanced version TFS2. With this logic, we will not need TFS2.

I'm not sure if any system is currently using TFS2. But technically, you need the code from the feature refactor + tfs3 to recreate the behaviour, so until that is merged too I would just keep it.

src/odemis/acq/move.py

src/odemis/model/_metadata.py

src/odemis/acq/move.py

install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml

pieleric · 2025-02-13T17:45:25Z

src/odemis/acq/move.py

+from odemis.util.transform import RigidTransform, _get_transforms
+
+# feature flags
+USE_3D_TRANSFORMS = False


I'd rather not have "feature flags". It introduces extra complexity (especially over time, eg, which version has which feature). Either the code is implemented, and the code is used, or it shouldn't be merged yet.

Let's just use 3D transforms all the time.

I don't think we can take such a simplified view of feature flags, especially when this is a smaller part of multiple pull requests to come. I've moved the flags into the MD_CALIB so they can be changed from the yaml file. The flags will be removed all togehter once all the features have been tested together more extensively

pieleric · 2025-02-13T17:56:30Z

src/odemis/acq/move.py

+
+# feature flags
+USE_3D_TRANSFORMS = False
+USE_SCAN_ROTATION = False


It shouldn't be a constant. I assume here this is about using 180° scan rotation on the e-beam scanner to match the FIB imaging angle. This will be used on the SEM coordinate referential transformation? Ideally, each microscope type (eg tescan_1, tfs_3) should either use or not scan_rotation. Alternatively, it would be a flag (using metadata) on the e-beam component. Pick your preference.

this is just a flag to enable the scan rotation correction. the code to calculate the transform reads the scan rotations directly

src/odemis/acq/move.py

pieleric · 2025-02-20T15:29:45Z

src/odemis/acq/move.py

+        """Move the stage vertically in the chamber. This is non-blocking.
+        From OpenFIBSEM"""


This function is quite difficult to grasp. It needs more info. In particular, when mentioning a vertical move, I think typically one would expect a "z" value. However, only "x" and "y" are used. This should be explained. Also, for relative moves, normally the argument is called "shift". Please use that name here too, because "pos" sounds too much like a "target position" / "absolute position".

Could you explain in the docstring how Y (in input) is related to a Z movement (in chamber referential)? Looking at the code, if rx = 0, then dy == 0 and dz == Y... which gives me the feeling that actually the two shifts input are x & z (in chamber referential). Is that correct? (if so, the key should be renamed "y" -> "z").

I've updated it to take x,z input shift and added more info to the docstring. x,y refer to the fact that this movement is calculated form the image view. Givne where this is, it's clearer that it is x,z

tepals

Some minor comments already, I'll do a more in depth review tomorrow

install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml

tepals · 2025-02-25T13:07:48Z

install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml

+        port: "/dev/fake6",
+        address: null,
+        axes: ["x", "y", "z", "rx", "rz"],
+        ustepsize: [1.e-6, 1.e-6, 1.e-6, 1.2e-5, 1.2e-5], # unit/µstep


I guess this is just a copy paste from other yaml files, but shouldn't this be unit/step, instead of unit/µstep?

tepals · 2025-02-25T19:21:47Z

src/odemis/acq/move.py

+            md = stage_md[model.MD_FAV_MILL_POS_ACTIVE]
+            rx = calculate_stage_tilt_from_milling_angle(milling_angle=md["rx"],
+                                                        pre_tilt=self.pre_tilt,
+                                                        column_tilt=math.radians(52))


why is this 52 degrees, does it make sense to have this defined as a constant somewhere?

tepals · 2025-02-25T19:22:24Z

src/odemis/acq/move.py

+        self._transforms[MILLING] = self._transforms[SEM_IMAGING]
+        self._inv_transforms[MILLING] = self._inv_transforms[SEM_IMAGING]
+
+        # add unknown as same as SEM IMAGING


can you extend the comment to explain why?

src/odemis/acq/move.py

tepals · 2025-02-25T19:32:58Z

src/odemis/acq/move.py

+            return self.to_sample_stage_from_stage_position2(pos)
+
+        # Convert position dict from dependant axes to original axes
+        # vpos = self._convert_to_sample_stage_from_stage([pos[self._axes_dep["x"]], pos[self._axes_dep["y"]]])


I think this line can be removed

tepals · 2025-02-26T09:46:06Z

src/odemis/acq/move.py

+
+        return transformed_pos
+
+    def _transformFromMeteorToSEM(self, pos: Dict[str, float]) -> Dict[str, float]:


can you make the names of the methods in this class consistent? You now have _transformFromMeteorToSEM and _transform_from_fm_to_fib, so a mix of Meteor/FM and camel/snake case

src/odemis/acq/move.py

Co-authored-by: Thera Pals <46314774+tepals@users.noreply.github.com>

tepals · 2025-02-27T09:58:15Z

src/odemis/acq/move.py

+                                 MILLING: tf_sr,
+                                 UNKNOWN: tf_sr}
+
+    def _get_scan_rotation_matrix(self) -> Tuple[numpy.ndarray, numpy.ndarray]:


This doesn't return anything

Suggested change

def _get_scan_rotation_matrix(self) -> Tuple[numpy.ndarray, numpy.ndarray]:

def _get_scan_rotation_matrix(self):

tepals · 2025-02-27T10:19:53Z

src/odemis/acq/move.py

+
+        logging.info(f"Position Posture: {POSITION_NAMES[position_posture]}, Target Posture: {POSITION_NAMES[posture]}")
+
+        self._posture_transforms = {


why do you have to update this dict in this function?

tepals · 2025-02-27T10:32:04Z

src/odemis/acq/move.py

+        self.required_keys.add(model.MD_CALIB)
+        self.check_stage_metadata(required_keys=self.required_keys)
+        self.check_calib_data(required_keys={model.MD_SAMPLE_PRE_TILT, "dx", "dy"}) # TODO: add "SEM-Eucentric-Focus"
+        if not {"x", "y", "rz", "rx"}.issubset(self.stage.axes):


why not check if "z" is there?

tepals · 2025-02-27T10:38:34Z

src/odemis/acq/move.py

+                        model.MD_STAGE_POSITION_RAW: pos_dep}
+                        )
+            except Exception as e:
+                logging.error("Failed to update %s with new position: %s", a, e)


I think a is undefined, is this log correct?

tepals · 2025-02-27T10:40:22Z

src/odemis/acq/move.py

+                f = ebeam_focus.moveAbs({"z": self.sem_eucentric_focus})
+                f.result()
+            except Exception as e:
+                logging.error("Failed to update ebeam-focus with new position: %s", e)


don't you want to log the position?

Suggested change

logging.error("Failed to update ebeam-focus with new position: %s", e)

logging.error("Failed to update ebeam-focus with new position: %s", {"z": self.sem_eucentric_focus})

tepals · 2025-02-27T13:19:25Z

src/odemis/acq/test/move_test.py

+        ssp3 = self.pm._get_sample_pos(stage_pos)                       # old 2D method
+
+        # assert near Position
+        self.assertTrue(isNearPosition(ssp, ssp2, axes={"x", "y", "z", "rx", "rz"}))


you can also use odemis.util.testing.assert_pos_almost_equal for testing positions

tepals · 2025-02-27T13:25:48Z

src/odemis/acq/test/move_test.py

+        f = self.stage_bare.moveAbs(self.stage_grid_centers[POSITION_NAMES[GRID_1]])
+        f.result()
+        f = self.pm.cryoSwitchSamplePosition(SEM_IMAGING)
+        f.result()


Can you add a comment to explain why you first need to move to GRID_1?

patrickcleeve2 requested review from pieleric and K4rishma January 23, 2025 07:35

patrickcleeve2 mentioned this pull request Jan 24, 2025

[refactor] [METEOR-1186] CryoFeature Refactor #3012

Open

K4rishma reviewed Jan 31, 2025

View reviewed changes

patrickcleeve2 force-pushed the meteor-960-multi-posture-imaging branch from 7833e22 to 853bd84 Compare February 3, 2025 08:04

patrickcleeve2 requested a review from K4rishma February 3, 2025 08:05

K4rishma reviewed Feb 3, 2025

View reviewed changes

src/odemis/acq/move.py Outdated Show resolved Hide resolved

patrickcleeve2 force-pushed the meteor-960-multi-posture-imaging branch from 853bd84 to c264604 Compare February 4, 2025 04:50

patrickcleeve2 requested a review from K4rishma February 4, 2025 05:35

K4rishma reviewed Feb 4, 2025

View reviewed changes

src/odemis/model/_metadata.py Outdated Show resolved Hide resolved

K4rishma approved these changes Feb 4, 2025

View reviewed changes

patrickcleeve2 force-pushed the meteor-960-multi-posture-imaging branch 2 times, most recently from bbe92e3 to 0d4855f Compare February 5, 2025 05:00

pieleric requested changes Feb 13, 2025

View reviewed changes

patrickcleeve2 force-pushed the meteor-960-multi-posture-imaging branch from 0d4855f to deeb427 Compare February 14, 2025 06:52

patrickcleeve2 requested a review from pieleric February 14, 2025 07:16

patrickcleeve2 mentioned this pull request Feb 18, 2025

[METEOR-960] FIBSEM GUI Preparation #2998

Open

pieleric requested changes Feb 20, 2025

View reviewed changes

[feat] add tfs3 posture manager for multi-posture imaging

f844134

patrickcleeve2 force-pushed the meteor-960-multi-posture-imaging branch from 755927d to f844134 Compare February 20, 2025 23:44

patrickcleeve2 requested a review from pieleric February 20, 2025 23:56

tepals reviewed Feb 26, 2025

View reviewed changes

patrickcleeve2 and others added 3 commits February 27, 2025 12:40

Update meteor-fibsem-sim.odm.yaml

6591dcb

Co-authored-by: Thera Pals <46314774+tepals@users.noreply.github.com>

Update move.py

de4016e

Co-authored-by: Thera Pals <46314774+tepals@users.noreply.github.com>

Update move.py

becc57f

Co-authored-by: Thera Pals <46314774+tepals@users.noreply.github.com>

tepals requested changes Feb 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat][METEOR-996] Add TFS3 posture manager for multi-posture imaging #3005

[feat][METEOR-996] Add TFS3 posture manager for multi-posture imaging #3005

patrickcleeve2 commented Jan 23, 2025 •

edited

Loading

K4rishma commented Jan 31, 2025

K4rishma Jan 31, 2025

K4rishma Jan 31, 2025

patrickcleeve2 Feb 3, 2025

K4rishma Jan 31, 2025 •

edited

Loading

patrickcleeve2 Feb 3, 2025 •

edited

Loading

patrickcleeve2 commented Feb 3, 2025

pieleric Feb 13, 2025

patrickcleeve2 Feb 14, 2025

pieleric Feb 13, 2025

patrickcleeve2 Feb 14, 2025

pieleric Feb 20, 2025

patrickcleeve2 Feb 20, 2025

tepals left a comment

tepals Feb 25, 2025

tepals Feb 25, 2025

tepals Feb 25, 2025

tepals Feb 25, 2025

tepals Feb 26, 2025

tepals Feb 27, 2025

tepals Feb 27, 2025

tepals Feb 27, 2025

tepals Feb 27, 2025

tepals Feb 27, 2025

tepals Feb 27, 2025

tepals Feb 27, 2025

		"""Move the stage vertically in the chamber. This is non-blocking.
		From OpenFIBSEM"""


		return transformed_pos

		def _transformFromMeteorToSEM(self, pos: Dict[str, float]) -> Dict[str, float]:

	def _get_scan_rotation_matrix(self) -> Tuple[numpy.ndarray, numpy.ndarray]:
	def _get_scan_rotation_matrix(self):


		logging.info(f"Position Posture: {POSITION_NAMES[position_posture]}, Target Posture: {POSITION_NAMES[posture]}")

		self._posture_transforms = {

	logging.error("Failed to update ebeam-focus with new position: %s", e)
	logging.error("Failed to update ebeam-focus with new position: %s", {"z": self.sem_eucentric_focus})

[feat][METEOR-996] Add TFS3 posture manager for multi-posture imaging #3005

Are you sure you want to change the base?

[feat][METEOR-996] Add TFS3 posture manager for multi-posture imaging #3005

Conversation

patrickcleeve2 commented Jan 23, 2025 • edited Loading

K4rishma commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K4rishma Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

patrickcleeve2 Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

patrickcleeve2 commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tepals left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickcleeve2 commented Jan 23, 2025 •

edited

Loading

K4rishma Jan 31, 2025 •

edited

Loading

patrickcleeve2 Feb 3, 2025 •

edited

Loading