Conversation
nvaytet
left a comment
nvaytet
left a comment
There was a problem hiding this comment.
I don't understand why CI is failing... a vector coordinate mismatch does not seem to originate from these changes...
| email-validator==2.3.0 | ||
| # via scippneutron | ||
| essreduce==26.1.1 | ||
| essreduce==26.3.1 |
There was a problem hiding this comment.
Should this be set as a lower bound in the pyproject.toml?
jl-wynen
force-pushed
the
lookup-error-threshold
branch
from
9a0b187 to
edb8ec9
Compare
This is related to a change in ESSreduce. Not sure yet which, but the coord transform from time to wavelength no longer removes the position. Do you know why? |
|
I now tracked it down to scipp/essreduce#302 |
|
My guess is that we now assign da.transform_coords(
'wavelength', graph={"wavelength": wavelength_from_tof}, keep_intermediate=False
)here. Because the Ltotal is already present, the |
|
Suggestion for a fix: we manually remove the |
|
See #244 |
…as not needed in the first place after we introduced WavelengthMonitor in generic workflow. That provider can be removed once we fix it in essreduce
| """I(Qx, Qy) with background (given by I(Qx, Qy) of the background run) subtracted""" | ||
|
|
||
|
|
||
| class WavelengthMonitor( |
There was a problem hiding this comment.
Instead, we can just use the WavelengthMonitor from essreduce.
| ) | ||
|
|
||
|
|
||
| def monitor_to_wavelength( |
There was a problem hiding this comment.
This provider was not really needed in the first place. We could have just used the one from essreduce that provides WavelengthMonitor. We now use it to 'fix' the monitors (by stripping the position coord).
The entire provider can be removed once we fix this in essreduce.
| use the ``WavelengthMonitor`` provided by the generic essreduce workflow. | ||
| """ | ||
|
|
||
| out = monitor.transform_coords('wavelength', graph=graph, keep_intermediate=False) |
There was a problem hiding this comment.
Note here that we went from keep_inputs=False (before) to keep_intermediate=False (now).
The input now has both position and Ltotal coords. With keep_inputs=False, the Ltotal would disappear and the position would remain.
Now, keep_intermediate=False removes nothing (maybe we don't need the kwarg at all?), and we manually remove position to only leave Ltotal at the end.
|
@jl-wynen can you take another look to check I didn't mess anything up? |
jl-wynen
left a comment
jl-wynen
left a comment
There was a problem hiding this comment.
Looks good. But I can't approve myself because I opened this PR,
2 participants
Adapt the
LookupTableRelativeErrorThresholdto be defined per component, as per scipp/essreduce#319Also:
Fixes #244