It's not the mechanism we talked about in standup, because that would require some pretty big changes to the workflow

Can you clarify? I thought all that is required is to modify add_coords_masks_and_apply_corrections to skip corrections that are None (or some other special value), and update the domain types to support that.

add_coords_masks_and_apply_corrections does not depend on correction factors that are computed in other providers. The correction factors are computed in the mentioned provider.
Changing that is by itself is quite a significant change.

add_coords_masks_and_apply_corrections does not depend on correction factors that are computed in other providers. The correction factors are computed in the mentioned provider. Changing that is by itself is quite a significant change.

Oh, what you are saying is that correction-computation is not part of the graph, right. Can you clarify why are you making this "quick" fix now instead of doing it properly? Is it needed for something right now?

Oh, what you are saying is that correction-computation is not part of the graph, right. Can you clarify why are you making this "quick" fix now instead of doing it properly? Is it needed for something right now?

Because I'm not sure what you mean by "doing it properly". I don't find it as clear cut as you seem to do. Like I said in the chat, it's probably best to talk in person.

If we make the footprint correction a separate domain type then we need to be able to access coordinates (specifically theta) in the provider that computes the footprint correction.
To access theta in that provider it either, needs to depend on a domain type that represents an event list that has the theta coordinate, or the theta coordinate needs it's own separate domain type.
If we make a separate domain type for the theta coordinate then that coordinate is computed separately from the other coordinates, and any time you need to access that coordinate you have to remember that it doesn't live on the event data but is it's own separate type, and it seems to me quite confusing.
So my preferred solution would be to have a domain type for an event list with all coordinates, but without correction factors applied to the weights.
But this collides a bit with the reduction workflow guidelines, because it does not have such a domain type.

Since there seems to be some disagreement on the right approach I went for what seems to me the solution that requires the least changes to the workflow, to avoid making a big refactor and then reverting to something different later.

I've been thinking about this a bit more, and I think the argument about separating theta as a separate domain type to make streaming more efficient is not a very good one in this case.

Because, the Estia detector is relatively small, right now it's 90K pixels, eventually it will be 180K pixels. An operations like sin(x)/x where x is a scipp variable with 180K entries it takes ~270us on my laptop. That is <0.5% of the total streaming time budget assuming we run the computation at 14Hz. In other words, it's too little to have any significant impact on the streaming performance.

The other argument for extracting the purely pixel dependent coordinates into separate domain types is that it would allow users to compute those quantities without computing the events, which might not be possible in some debugging scenarios.
But in my opinion that is easy enough to do manually already:

# Here's how I imagine it should be done.
res = workflow.compute((CoordinateTransformationGraph, EmptyDetector))
res[EmptyDetector].transform_coords(['theta', 'dirvergence_angle'],  graph=res[CoordinateTransformationGraph])

so I don't think making this simpler is worth it.

Extracting the purely pixel dependent coordinates theta and divergence_angle would make some coordinates live outside of the event data DataArray, and some would live inside it, and I think that complication is not worth the minor performance improvements in the streaming case.

So I'm suggesting that we go with the currently implemented approach for the time being.