Units for detector.get_beam_position

[rhfogh]

We need to standardise on units for the detector.get_beam_position function. Without defined units you do not know what the return values mean, and you cannot use the function except by implicitly assuming which beamline it is being run on.

In AbstractDetector the function documentation says "Beam position x,y coordinates [pixel]."

Yet the ESRF function returns values in mm. It is tricky to see, since the actual values are determined by the configuration values of ax, ay, bx, and by, but it looks like lnls also uses mm, whereas SOLEIL uses pixels.

Whatever it currently says in AbstractDetector, we can agree to use pixels or mm, or to have both get_beam_position_mm and get_beam_position_pixel, or probably other solutions, but could we agree on a single convention?

[marcus-oscarsson]

@rhfogh the code you are referring to is beamline specific and still in the process of being adapted to mxcubecore hence some inconsistencies.

I'm otherwise in favor of using device independent values for instance SI or more domain specific units when appropriate like microns or angstrom so I would suggest millimeters for get_beam_position.

I thought that was already the decision, but maybe I'm mistaken ?

Why not adding the <unit>_ suffix if we deviate from the above (if that's what we decided) and the documentation should of course be correct regardless of suffix.

I have a vague recollection though of a discussion were we might have thought pixels were a better return value for get_beam_position. @MartinSavko was maybe involved in that ?

Marcus

[marcus-oscarsson]

After discussing this with @rhfogh, we agreed that we would stick to the current definition of AbstractDetector and raise this more as a topic to discuss on our next developers meeting

[rhfogh]

Opened temporarily as resource for discussion in MXCuBE developers' meeting

[MartinSavko]

My memories of the discussion on the topic are probably very much clouded by my slight bias towards wanting .get_beam_position() to return in pixels (and nexus documentation recognizes it as an allowed one). My preference for this is probably just due to the feeling that the pixel is the natural unit of a digital image.

Other than that I would support get_beam_position_mm() and get_beam_position_pixels().

The question of related methods get_radius() and get_outer_radius() is very illuminating. There I have a feeling mm is a more "natural" unit.

To avoid confusion we could append _pixels and _mm to all those methods and add add a 'unit' keyword argument to the default method?

e.g. def get_beam_position(self, unit='pixels'): if unit='mm': return self.get_beam_position_mm() return self.get_beam_position_pixels()

def get_radius(self, unit='mm'): if unit='mm': return self.get_radius_mm() return self.get_radius_pixels()

[marcus-oscarsson]

@rhfogh, All

So I looked at this a bit further as we are implementing this at the moment. There might be a conversion missing in get_beam_position() depending on how want wants to configure beam center.

` try: distance = distance or self._distance_motor_hwobj.get_value() wavelength = wavelength or HWR.beamline.energy.get_wavelength() metadata = self.get_metadata()

        beam_position = (
            float(distance * metadata["ax"] + metadata["bx"]),
            float(distance * metadata["ay"] + metadata["by"]),
        )
    except (AttributeError, KeyError):
        beam_position = (None, None)

    return 

`

As far as I can tell the beam position can't be anything else than millimeters using the calculation above as long as the pixel to millimeters conversion is not included in ay and ax.

Would it not make more sense to use millimeters for this interpolation and simply convert the return value to pixels ?

[rhfogh]

Well, it all depends on what values you use for ax, ay, bx, and by. As far as I am aware it is not explicit anywhere what the unit of those values should be - they are in pixels at SOLEIL. In fact the lack of specification is arguably a weakness of the current system - the only 'specification' is in the fact that they must match the code that uses them. And that code is different in develop and in the MASSIF1 production code. In the develop branch the relevant code in abstract/AbstractDetector.get_radius is:

    beam_x, beam_y = self.get_beam_position(distance)
    pixel_x, pixel_y = self.get_pixel_size()
    rrx = min(self.get_width() - beam_x, beam_x) * pixel_x
    rry = min(self.get_height() - beam_y, beam_y) * pixel_y
    radius = min(rrx, rry)

which presupposes a position in pixels. It looks to me like the code in get_radius was changed to match an input in mm.

We could consider making get_beam_position an abstract method (in pixels) and removing all references to ax,ay,bx,by from the abstract class. More coding, but less scope for confusion. But failing that we should probably try to enforce the correct nits for ax, ay, bx, by.

[marcus-oscarsson]

I don't think there is any added value in abstracting away the interpolation for the beam position so that ax, by, bx and by can be on either pixels or mm

Actually I think it would be a good thing if we could agree on the unit for those configuration/calibration parameters. The code as it is today somehow assumes that its in pixels.

I have not really anything against this, I just found it a bit awkward to perform an interpolation with a distance in mm as input and calibration constants defined in pixels.

[marcus-oscarsson]

So we decided during the meeting yesterday that we would keep the interface as it is in AbstractDetector.

The interface defined in AbstractDetector more or less forces us to use pixels in the detector configuration file. I don't really have anything against this, and it would in the end be a good thing if pixels could be used consistently across all sites.

It more or less only leaves one question left and that is what unit should be used for the beam center value in ISPyB ?

I think it's even more important that we are consistent across sites when it comes to this value. We currently use millimeters here at ESRF.