Convert reduced data back to tof #106
Conversation
jl-wynen
force-pushed
the
back-to-tof
branch
from
62078a2 to
acad30d
Compare
jl-wynen
force-pushed
the
back-to-tof
branch
from
78d6ed5 to
3d370a3
Compare
src/ess/powder/conversion.py
Outdated
| ).value | ||
| return OutputCalibrationData( | ||
| sc.DataArray( | ||
| sc.array(dims=['calibration'], values=[difc]), |
There was a problem hiding this comment.
Why are why taking the value (stripping units?) and put it back into a variable?
There was a problem hiding this comment.
Because the underlying CIF writer wants the coefficients as a data array (https://scipp.github.io/scippneutron/generated/modules/scippneutron.io.cif.CIF.html#scippneutron.io.cif.CIF.with_powder_calibration):
cal = sc.DataArray(
sc.array(dims=['cal'], values=[3.4, 0.2]),
coords={'power': sc.array(dims=['cal'], values=[0, 1])},
)It is unfortunate that we have to remove the unit but that is how CIF wants the data.
We could instead keep a dict until the last moment where we need to convert to the above data array:
{
0: sc.scalar(3.4, unit='us'),
1: sc.scalar(0.2, unit='us/angstrom')
}Would you prefer this?
There was a problem hiding this comment.
I don't mind much, but can you explain what is going on in the comments?
There was a problem hiding this comment.
Have a look at the new implementation.
src/ess/powder/conversion.py
Outdated
| def convert_to_calibrated_tof( | ||
| data: IofDspacing, calibration: OutputCalibrationData | ||
| ) -> IofTof: | ||
| difc = calibration['power', sc.scalar(1)].data |
There was a problem hiding this comment.
Is the plan to have other powers as well? Is a DataArray the right data structure here? Maybe a DataGroup or simple dataclass would be more intuitive?
There was a problem hiding this comment.
Is the plan to have other powers as well?
Ultimately, yes, as far as I know. @celinedurniak ?
See #106 (comment) why it is a data array. A data group would not work because the keys should be powers (int) and not names (str). A dataclass would have a fixed set of fields. Plus it would also need names instead of powers.
There was a problem hiding this comment.
@jl-wynen your assumption is correct. There could be, for example, these terms to consider
difc * dspacing + difa * dispacing **2 + difb/dspacing +Zero
There was a problem hiding this comment.
Ok. Do we need to support them now or can this wait until we actually need them?
src/ess/powder/conversion.py
Outdated
| data: IofDspacing, calibration: OutputCalibrationData | ||
| ) -> IofTof: | ||
| difc = calibration['power', sc.scalar(1)].data | ||
| difc.unit = 'us / angstrom' |
There was a problem hiding this comment.
We stripped the unit in the function above, and now we put it back?
src/ess/powder/conversion.py
Outdated
| res = data.drop_coords('dspacing').bins.drop_coords('dspacing') | ||
| res.coords['tof'] = sc.to_unit(difc * data.coords['dspacing'], unit='us') | ||
| res.bins.coords['tof'] = sc.to_unit(difc * data.bins.coords['dspacing'], unit='us') |
There was a problem hiding this comment.
I presume you have considered using transform_coords but decided to do this manually?
There was a problem hiding this comment.
I hadn't. Becuase the callibration coefficients are not coordinates. I could use a closure to capture them as a transformation function?
There was a problem hiding this comment.
It would avoid having to manually handle dense and binned coords. Give it a try, but this is also fine?
THis allows us to keep the units of the coefficients
src/ess/powder/calibration.py
Outdated
| ) | ||
|
|
||
|
|
||
| OutputCalibrationData = NewType('OutputCalibrationData', ScalarCalibrationData) |
There was a problem hiding this comment.
Can there be different ones? Why do we need the alias?
Fixes #97