Add AbstractDoseEstimation

[mockoocy]

This is part of the dose estimation capability that we have presented at the developer's meeting in Berlin

This HardwareObject is meant to provide a way to estimate absorbed
dose by crystal during oscillation scans.

Includes abstract definition along implementations at BioMAX and a mockup one.

Note on the BioMAX implementation:
Calculations inside the module have been based on the old dose estimation
code used at BioMAX and is a subject to change in the future.
It's correctness is not guaranteed, nonetheless it serves as a reference
implementation to iterate on afterwards.

[rhfogh]

Hmmm. This may be mostly a question of style, but I would not be that happy having to write an implementation to this abstract class. Could we discuss this?

Mainly I think that this very much exaggerates using Pydantic. First of all DoseEstimateParameters. The normal Python mechanism is that function parameters are defined in the function definition. Here they are defined in a Pydantic class, in a different file. Does that not obfuscate more than it helps? There are cases where data objects make sense, e.g. for the data needed to define an acquisition, that have some kind of independent objecthood and that are used in multiple contexts. But these are (just look at the name) defined simply by being the input parameters to a function. Why the extra packaging?

Similarly the various classes for DoseEstimation results and goals. What is gained by this rather cumbersome modelling just to group a few attributes together?

Are we aiming for a situation where the input and output of every function in MXCuBE is separately defined as a Pydantic object in a separate file? Is that really a good idea? And if that is not what we want, how do you decide which contexts can make do with standard Python, and which need the all-Pydantic treatment? I am generally in favour of data modelling, but part of the cost is that it takes more thought and work to set up a comprehensive model up front, and leads to more rigid code. What is the gain, here?

[mockoocy]

Why the extra packaging?

The Pydantic model is used in a few places, one of them being the backend adapter, which essentially calls the estimate_dose function with the same params object. It is necessary there because it validates the arguments coming from the form.

Using the same object in mxcubecore was mainly a matter of convenience, although I agree that this may introduce too much coupling to Pydantic. The same argument applies to the return types: these Pydantic models make the results very convenient for the frontend to consume, although mxcubecore should not really care about what the frontend prefers…

It also occurs to me now that the name DoseEstimationParameters may be too generic. The object specifically carries parameters supplied by the user. We may need to distinguish between what the user wants to set and the current beamline state. For example, the BioMAX implementation uses both the user-supplied energy and the current beamline energy to calculate an error-correction term for flux interpolation.

The Pydantic models could be removed from mxcubecore fairly easily, for example by using keyword-only arguments for estimate_dose, raising an exception such as DoseEstimationUnavailableError(...) for expected failures, and keeping the Pydantic models in mxcubeweb.

[rhfogh]

@mockoocy OK, that is a good explanation. We can still discuss how much the front end should influence the back end (and I still have my opinions, no surprise there) but there are certainly good reasons on both sides. Can we see what the others say, before this is merged? Meanwhile, I will remove my 'modifications required', so as not to be technically blocking.

I still think this should be discussed more before being merged (and my opinion is the same as in the previous review), but there are good arguments on both sides, so in the absence of a specific change I would rather not stand as blocking further developments.

[marcus-oscarsson]

Would it be possible to merge DoseEstimationOk and DoseEstimationError to simply DoseEstimation containing i.e status, msg, dose_mgy, max_images, the later two being None incase of error ? Or containing them within a field called result ?

[mockoocy]

I think I would just raise some named exception (e.g. DoseEstimationError) here and have a proper field with no None values there. Thus having just one return type (I'd make it a dataclass probably or a TypedDict for the sake of readability).

[marcus-oscarsson]

I would agree with @rhfogh for params, is there a reason for not simply defining the method with all the arguments needed ?

[mockoocy]

Just convenience when writing code for the web frontend, I will just use bunch of kwargs there, someting like

- def estimate_dose(self, params: DoseEstimateParameters): ...
+ def estimate_dose(self, *, exp_time_s: float, num_images: int, ...): ...

[marcus-oscarsson]

I'm not against using DoeseEsitmationError in this way, it can be an elegant way to handle an expected error condition. We tend to use exceptions for both "unexpected" and expected error states. Why did you chose to with dose DoeseEsitmationError ?

If you would like to distinguish between exception and an "expected" error. I would merge, as I mentioned above, DoseEstimationOk and DoseEstimationError.

Otherwise the easiest would perhaps be to raise and let the calling code handle the error ?

[mockoocy]

I will just go with something proposed in #1643 (comment) and raise indeed. It is a bit more "python-like" :), I will move the web niceties to mxcubeweb.

[marcus-oscarsson]

I understand the idea of using the PyDantic models like this to make the implementation in mxcubeweb easier/lighter. But as you both point out after the discussion above I also think that moving this part to mxcubeweb is maybe more appropriate. The method estimate_dose should take all the parameters needed for the estimation, any extra argument can be passed as kwarg or handled in some site specific override.

This otherwise looks great to me :), @rhfogh, is there otherwise anything else in the structure that you would like to discuss.

From what I see, I think we can merge this if the PyDantic bits we mentioned above an be moved to mxcubeweb ?

[rhfogh]

If the pydantic stuff is moved to mxcubeweb Iit is all fine with me. Not that I really like the style, but the web front end has its own legitimate requirements.

[beteva]

Sorry, coming a bit late in the discussion (long holidays). In order to agree on the parameters and what comes from the form, hence the pydantic model, could we narrow down the number of parameters. It seems to me that we can estimate the dose, based on the dose rate and the total exposure time. Is this too simplistic?

[rhfogh]

Could we integrate this with the dose rate PR, #1619. I particularly would point to the conceptual organisation proposed in recent comments.

[mockoocy]

Hi all,
sorry for late reply, but I was holiday.

@rhfogh: Could we integrate this with the dose rate PR, #1619. I particularly would point to the conceptual organisation proposed in recent comments.

I suppose it could be up to facility to use HWR.beamline.dose_rate in their estimate_dose implementation? I think we could just leave AbstractDoseEstimator oblivious to other hardware objects and probably have some default implementation that would return HWR.beamline.dose_rate * exp_time_s * num_images.
Could be part of separate PR / bundled with #1619.

@beteva: Sorry, coming a bit late in the discussion (long holidays). In order to agree on the parameters and what comes from the form, hence the pydantic model, could we narrow down the number of parameters. It seems to me that we can estimate the dose, based on the dose rate and the total exposure time. Is this too simplistic?

Yes, in principle, given dose rate it is quite simple to get the proper dose estimation later on.
The parameters that are passed into the function that does the estimation would correspond to what user set in the "standard" Data Collection form on the frontend. That is -- they may want to change some parameters (e.g. transmission, energy) and different dose rate (and subsequently dose estimate) could be then expected. In other words, the dose estimate refers to a "requested" beamline state rather than the current one (thus the need to tell what would be expected).

For example, BioMAX implementation tries to estimate flux at energy level provided by the user. To do so it calculates a relative error term between a real flux value and one coming from some cubic regression. This is then used to infer what would be the flux quantity given user-supplied energy.

I see a reason that we could perhaps just operate on beamline values and just prohibit dose estimation when user requests different energy than the one flux has been measured for. It could probably save us some pain, and we could safely just send transmission, num_images, exp_time_s to mxcubecore.

Sorry for this long wall of text, I hope it is digestible :-)

[rhfogh]

I think the approach here is not ideal because it tries to put the entire calculation into a single function/object. The problem with that is that if at some later date you want to make a change to some of the parts or use different assumptions it all gets very complicated, as you would have to decompose it. I would propose the following:

The top formula is
dose = get_dose_rate_per_photon_per_mmsq * effective_flux_density * relative_crystal_sensitivity * time
That should be simple enough that you could inline it wherever you want and would not need a separate function for it. We could then have separate, overridable functions to calculate each of the four terms in the equation, and put those functions into abstract objects as we find best.

We would need functions:
get_dose_rate_per_photon_per_mmsq(energy)
Which is a conversion factor that depends only on the energy.

get_effective_flux_density(transmission=None)
The effective flux density could in principle vary from point to point in the crystal. If you use the average flux density, it depends on transmission, flux, and beam area, and if there is a need for it all of these could optionally be passed in (defaulting to the current values) as in
get_effective_flux_density(transmission=None, flux=None, beam_area=None)

See the current AbstractFlux class for the current implementation of these two functions.

If we ever get to using the real (non-top-hat) beam profile and the actual crystal shape to calculate dose, the effective flux density would get a lot more complicated, and there would have to be operational decisions as to which value to use, or whether to calculate some kind of weighted integral. For a simple case (large crystal, narrow beam, approximation) see GphlWorkflow.maximum_dose_rate(). Anyway, this being so dependent on assumptions it might be better not to put it into a top-level combined function (I am quoting Gleb Bourenkov here)

The relative crystal sensitivity is in principle wavelength dependent. For now it can be assumed to be a constant passed in with the diffraction plan (I am not aware of any better implementation).

The relevant time is easy to calculate, and can be left to be done locally.

I think this would give a nice, clean separation between the various terms and make it easier to understand and maintain. Note that we might have not only calculations from crystal shape but also multi-sweep, multi-wavelength experiments, so it is better not to bundle all the calculations up.