Update for next version of Sciline #63
Conversation
pyproject.toml
Outdated
| "plopp", | ||
| "pythreejs", | ||
| "sciline>=23.9.1", | ||
| "sciline @ git+https://github.com/scipp/sciline@main", |
There was a problem hiding this comment.
As I told Johannes, I am not happy with depending on an unreleased version. Especially on a moving target, i.e., a branch, not a concrete commit.
Can we make a Sciline release now / soon before merging this PR?
There was a problem hiding this comment.
Sure that is the plan, this is temporary to pass CI.
src/ess/powder/masking.py
Outdated
| return {key: value for d in dicts for key, value in d.items()} | ||
|
|
||
|
|
||
| def set_pixel_mask_filenames( |
There was a problem hiding this comment.
| def set_pixel_mask_filenames( | |
| def with_pixel_mask_filenames( |
because it does not modify the input.
There was a problem hiding this comment.
@nvaytet Should we change all the naming of similar functions in ESSsans?
| " TofMask: lambda x: (x < sc.scalar(0.0, unit=\"ns\"))\n", | ||
| " | (x > sc.scalar(86e6, unit=\"ns\")),\n", | ||
| " TwoThetaMask: None,\n", | ||
| " WavelengthMask: None,\n", |
There was a problem hiding this comment.
Can we have default params so users don't have to specify everything? I thought that was the plan anyway?
There was a problem hiding this comment.
Yes, but as far as I could tell ESSdiffraction is not implementing C.5 yet (see https://github.com/scipp/essreduce/blob/main/docs/user-guide/reduction-workflow-guidelines.md#c5-use-a-fixed-pattern-for-creating-manipulating-and-running-workflows), so this is unrelated to this PR?
| "\n", | ||
| "pipeline = sciline.Pipeline(providers, params=params)" | ||
| "pipeline = sciline.Pipeline(providers, params=params)\n", | ||
| "pipeline = powder.set_pixel_mask_filenames(pipeline, [])" |
There was a problem hiding this comment.
It is not great that this always needs to be called even when there are no file names. Can this be done with a default parameter?
There was a problem hiding this comment.
I agree, but it cannot, unfortunately (since the Optional this is replacing is a couple of nodes downstream of the filename).
Alternatives:
- Add parameter to the workflow creation, such
DreamWorkflow(pixel_masks=[]). - Set a default for
DetectorMasks, which has the pitfall that later settingPixelMaskFilenamesilently has no effect (since setting the default forDetectorMaskstrimmed the branch of providers leading toPixelMaskFilename.set_pixel_mask_filenamescould re-add this branch, but the pitfall remains.
There was a problem hiding this comment.
What about setting PixelMaskFilename = [] by default?
There was a problem hiding this comment.
You mean as a default param in DreamWorkflow? That can be done, but mapping again differently afterwards is not possible.
|
As agreed elsewhere, I will merge and release. Main comments were addressed, other things (such as masking UX) can be improved later, once we have a better idea. |
The main change is around the new way of handling
Optional. Pixel masks are now handled usingmap, which will also support multiple masks.Note that this is incompatible with the released Sciline, i.e., we will need to make a synchronized release.