Geometry update by motor positions of quadrants

[egorsobolev]

This implements the update of geometry according to the motors of detector quadrants. The motor position can be write to and read from Crystfel geometry files (write as comments). There is also the helper to read motors from experimental data.

@takluyver @philsmt @JamesWrigley

[takluyver]

Hi Egor, sorry it has taken me a while to look at this. Before I get into commenting on specific bits of the code, I have a few broader questions & thoughts.

Am I understanding correctly that we can only calculate quadrant shifts based on motor movements, not absolute positions? I.e. we can't translate motor positions directly to quadrant positions, we need a previous geometry and the difference in motor positions between the two?

I think the code to read motor positions from saved data (what's currently the read_motors_from_data() function) would be a more natural fit as an EXtra component which knows about the source name patterns - this is quite similar to the pattern of components we already have or have planned. It could look something like this:

from extra.components import AGIPDQuadrantMotors

quad_motors = AGIPDQuadrantMotors(run)
quad_motors.get_positions(atol=0.01)

I don't particularly like the idea of storing motor positions in comments in .geom files - both because it's essentially defining a new ad-hoc file format, and also because if someone modifies the geometry with a tool that ignores the motor positions, they can be out of sync with the module positions they're stored with. But I also don't really have a better idea for how to store these at the moment.

[egorsobolev]

Am I understanding correctly that we can only calculate quadrant shifts based on motor movements, not absolute positions? I.e. we can't translate motor positions directly to quadrant positions, we need a previous geometry and the difference in motor positions between the two?

Yes. But its generalization. If you wanna use absolute position, you just define the reference quadrant positions for zero motor positions:

    quad_pos_for_zero_motor_pos = [(-525, 625), (-550, -10), (520, -160), (542.5, 475)]

    geom = AGIPD_1MGeometry.from_quad_positions(quad_pos_for_zero_motor_pos)
    geom2 = geom.move_by_motors(motor_pos)

I think the code to read motor positions from saved data (what's currently the read_motors_from_data() function) would be a more natural fit as an EXtra component which knows about the source name patterns - this is quite similar to the pattern of components we already have or have planned. It could look something like this:

from extra.components import AGIPDQuadrantMotors

quad_motors = AGIPDQuadrantMotors(run)
quad_motors.get_positions(atol=0.01)

I have no objections. I put all in one place to start discussion. It would be super if it would not need source names. But they depends on the instrument. (yes, we can hard code different sources depending on agipd sources in a run)

I don't particularly like the idea of storing motor positions in comments in .geom files - both because it's essentially defining a new ad-hoc file format, and also because if someone modifies the geometry with a tool that ignores the motor positions, they can be out of sync with the module positions they're stored with. But I also don't really have a better idea for how to store these at the moment.

I also don't like. But I think its good enough for now until we find better way. I don't think that is the problem, because if someone modifies geometry with a tool which doesn't know about motors, than we don't know the reason of modification. It's better to discard the motors in this case. Then we need motors only for reference geometry, which instruments supposed to make and keep. Later, I hope, we will store reference geometries in caldb and find a way how to pack the motor positions there.

[egorsobolev]

What do you think about mixin implementation?

[takluyver]

Are those reference positions you gave real, and accurate enough to be useful? If we can convert absolute motor positions into absolute quadrant positions reliably, I think that opens the door to simpler interfaces, like:

quad_motors = AGIPDQuadrantMotors(run)

geom = AGIPD_1MGeometry.from_motor_positions(
    quad_motors.get_positions(atol=0.01), reference="MID",
)

[egorsobolev]

The simpler interface would be good. I gave the numbers from example, but there are real numbers for AGIPD@MID: [(-542, 660), (-608, -35), (534, -221), (588, 474)].

The problem that these numbers are not guaranteed. They can stay the same for any long time, but also may be changed any moment (for example by re-calibrating encoders, and that is not a single option). It means that it is not a good idea to hard code them in library. Thus, simpler interface can look:

quad_motors = AGIPDQuadrantMotors(run)

geom = AGIPD_1MGeometry.from_motor_positions(
    quad_motors.get_positions(atol=0.01),
    reference_quad_pos=[(-542, 660), (-608, -35), (534, -221), (588, 474)]
)

Which is actually shortcut replacing just two calls:

geom = AGIPD_1MGeometry.from_quad_positions(reference_quad_pos).move_by_motors(motor_pos)

Another problem, that ideal geometry doesn't work for SFX experiments, which are sensitive to the module rotations. That mean we cannot use ideal geometry and need to use refined stored in the file. But we potentially could refine geometry (if we do it with our tools) relative some hard coded origin (move there after refinement using motor positions during the calibration run, instead storing them in the geometry). But then we cannot distinguish geometry files which respect that convention and others.

I would go with explicit way to store and specify motor positions.

[takluyver]

Looking at this again:

• As before, I think the code to read motor positions from saved data fits better as an EXtra component rather than here. I can have a go at writing that if you like.
• I don't especially like mixins - I find it makes the inheritance trickier to think about. I think you can do this just as well with an intermediate class: AGIPD_1MGeometry -> DetectorWithMotors -> DetectorGeometryBase. Or even just do it for AGIPD-1M for now, and we can generalise later if we need to. However, this is just stylistic, so feel free to stick with a mixin if you prefer that.
• Reading the motor positions from the file should be in from_crystfel_geom rather than __init__. The __init__ method shouldn't assume that the filename it gets is a .geom file, and silencing any errors when reading the file is liable to make for confusing bugs.
• The geometry objects are meant to be basically immutable - public methods to modify the objects always return a new object rather than changing the existing one. As it stands, set_motor_positions & set_motor_axes break this pattern. I'd like them to either return new objects - (these could be named like with_motor_positions or replace_motor_positions) or become private methods with a _ prefix, if they're only meant for use in methods like .from_crystfel_geom.
• If the geometry doesn't have motor positions, move_by_motors will assume it has motors in the reference/zero positions, which seems wrong & confusing. If we don't know the motor positions to start with, trying to calculate a geometry from new motor positions should throw an error.
  • If we think people will often want to use movements - "motor XYZ was moved 2 mm from where it was just now" - rather than positions, maybe there should be a separate method for that.

[egorsobolev]

Looking at all suggestions, I decided to refactor it a bit:

• the code to read of motor positions from saved data goes to EXtra, see Detector motors component EXtra#148
• the code to update geometry goes in new class BaseMotorTracker and its implementation for specific detectors
• the motor positions stay in the geometry class
• reading the motor positions from the file goes in from_crystfel_geom, writing is in crystfel_fmt.py
• no one method updates geometry itself, they update BaseMotorTracker instead
• the methods, which update geometry or motor position in it, return new geometry instance
• there are two ways to create tracker, with and without reference motor positions
• there are two methods to update geometry geom_at and move_geom_by; the first one raises an exception if the tracker does not have reference motor positions, the second does not set motor_positions to the new geometry

Usage without reference motor positions:

    geom = AGIPD_1MGeometry.from_quad_positions(quad_pos)
    tracker = AGIPD_1MMotors(geom)
    geom2 = tracker.move_geom_by(motor_pos)

Usage with reference motor positions:

    geom = AGIPD_1MGeometry.from_quad_positions(quad_pos)
    tracker = AGIPD_1MMotors.with_reference_positions(geom)
    geom2 = tracker.geom_at(motor_pos)

    tracker = AGIPD_1MMotors.with_reference_positions(geom, ref_motor_pos)
    geom2 = tracker.geom_at(motor_pos)

[takluyver]

I think the structure of this looks good, thanks! I have a few comments about details above.

[takluyver]

Thanks, this LGTM now. Would you be able to write a bit of documentation before we merge it? Even just an example notebook showing how to use motors to adjust geometry?

[JamesWrigley]

I don't quite understand why we're storing motor positions in the .geom files? I assume the point of saving the metadata is for the sake of reproducibility/provenance; if so, don't we need both the motor positions and the reference geometry?

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[egorsobolev]

Thanks, this LGTM now. Would you be able to write a bit of documentation before we merge it? Even just an example notebook showing how to use motors to adjust geometry?

Sure. I've pushed it.

I don't quite understand why we're storing motor positions in the .geom files? I assume the point of saving the metadata is for the sake of reproducibility/provenance; if so, don't we need both the motor positions and the reference geometry?

The main goal is to store the refined geometry (by SFX data, for example) together with corresponding motor positions. At them moment we use crystfel geometry and sending the geometry files around. Thus, this solution seems natural and transparent.

This solution is not ideal (see the first comments from @takluyver), but there is no obvious better option for now. If we store geometry in CalDB, we likely can get rid of this hack.

[JamesWrigley]

The main goal is to store the refined geometry (by SFX data, for example) together with corresponding motor positions. At them moment we use crystfel geometry and sending the geometry files around. Thus, this solution seems natural and transparent.

Sure, but what exactly can you do with that? Do you plan to use them to uniquely identify a particular geometry, or are the positions used by crystfel or something? Sorry if this is a dumb question 😅 It's just not clear to me how the motor positions are useful without the reference geometry.

[egorsobolev]

But the reference geometry is in the same file.

We store refined geometry in a file (refined by special procedure using the SFX data but not motor changes) and also store corresponding motor positions there (motor positions at the moment when data for refinement were taken).

Then operators move motors and ask me to generate new geometry (for example). I read current motor position from Karabo devices, but I need the difference between current positions and positions which correspond to the refined geometry. The last are missed until I store them somewhere (better togehter with refined geometry, which is reference geometry now).

I have implemented writing and reading motor position in the geometry file especially for this particular case. In other cases, this is just a metadata which may be used or not.

For example, motor positions can be used to check if the geometry is still valid. You just compare motor positions stored in the geometry file and motor positions in data. If they are different, then you need to update geometry. For SFX it means full refinement

[egorsobolev]

if there are no more comments... do I merge?

[JamesWrigley]

But the reference geometry is in the same file.

We store refined geometry in a file (refined by special procedure using the SFX data but not motor changes) and also store corresponding motor positions there (motor positions at the moment when data for refinement were taken).

Ah I see, I misunderstood and thought that the idea was just to store the motor positions with the new geometry.

LGTM!

[egorsobolev]

@takluyver and @JamesWrigley, thank you both for review