refactor: use parameter models from esslivedata#654
Conversation
github-actions
Bot
added
essdiffraction
| DetectorBank.south: {'y': 200, 'x': 500}, | ||
| DetectorBank.north: {'y': 200, 'x': 500}, | ||
| }, | ||
| KeepEvents[RunType]: KeepEvents(False), | ||
| TwoThetaMask: lambda two_theta: ( | ||
| (two_theta < sc.scalar(float('inf'), unit=two_theta.unit)) | ||
| | (two_theta > sc.scalar(float('-inf'), unit=two_theta.unit)) | ||
| ), | ||
| TofMask: None, | ||
| WavelengthMask: None, | ||
| MaskedDetectorIDs: MaskedDetectorIDs({}), | ||
| NeXusDetectorName: "detector", | ||
| DetectorBank: DetectorBank.south, | ||
| Filename[SampleRun]: str(mcstas_powder_silicon_in_vanadium_can()), | ||
| Filename[VanadiumRun]: str(mcstas_powder_vanadium()), | ||
| Filename[EmptyCanRun]: str(mcstas_powder_empty_can()), |
There was a problem hiding this comment.
These changes will be revisited or removed.
They were added mainly for debugging purposes.
| stop=self.stop, | ||
| num=self.num_bins + 1, | ||
| unit=self.unit.value, | ||
| ) |
There was a problem hiding this comment.
Why does this have to be re-implemented here?
Looks like only the dim and descriptions should need to be specified.
| # TODO: for now, we leave the mapping over detector banks out, because we do not have a | ||
| # method to merge the I(Q) of different banks, and we thus cannot compute a single | ||
| # result from the workflow. So only a single detector bank can be processed at a time. | ||
| # parameter_mappers[NeXusDetectorName] = with_banks |
There was a problem hiding this comment.
Comment should not be removed.
| @dataclass(frozen=True) | ||
| class WorkflowSpec: | ||
| """Metadata needed to generate interfaces for a workflow.""" | ||
|
|
||
| name: str | ||
| title: str | None = None | ||
| description: str | None = None | ||
| version: str | None = None |
There was a problem hiding this comment.
Why does the WorkflowSpec not hold models for workflow inputs and outputs, or the param-specs you defined?
There was a problem hiding this comment.
Updated it so that now it does. I agree that makes more sense.
| @dataclass(frozen=True) | ||
| class ParameterSpec(Generic[T]): | ||
| """Specification for configuring a Sciline workflow key.""" | ||
|
|
||
| model: Any | ||
| category: str | ||
| title: str | None = None | ||
| description: str | None = None | ||
| default: T | KeepDefaultType = keep_default | ||
| transform: Callable[[T], Any] | None = None | ||
| apply: Callable[[Pipeline, T], Pipeline] | None = None | ||
| filter_keys: tuple[Key, ...] = () | ||
| use_workflow_default: bool = True | ||
| key: Key | None = None |
There was a problem hiding this comment.
This seems like a central part we should talk about. It seems like the idea is that a set of ParameterSpec would/could take the role of the input-params-model (and output-params-model)? One concern I have is that this ties the spec to the implementation (transforms, key, ...), which would force full dependencies in UI packages that do not run code. title, description, and default we'd get for free as field attributes if we used a model holding all params. On the other hand, storing them in this param spec seems like it has the advantage in composability, as we can provide multiple workflows with different subsets of param specs (exposed inputs).
There was a problem hiding this comment.
I think there are some suspect fields that were added by codex for quite hacky reasons, for example filter_keys. I'll try to iterate a bit on this to make it leaner.
But I don't really understand what you mean, how could the input params model not hold a reference to the sciline Key that it intends to represent? Or do you mean that the input parameters is another separate layer of input parameters that are then mapped to sciline keys in a separate stage? Or did you mean something different?
It's probably best to talk in chat. Do you have time at 13?
There was a problem hiding this comment.
In ESSlivedata the workflow factory is responsible for this. Only it know the implementation and how to map model fields to workflow keys (there may even be non 1:1 mappings), the model does not.
|
|
||
| # TODO: the mask parameters (TofMask, TwoThetaMask, WavelengthMask) need a new widget | ||
| # that allows to define a python function in the notebook and pass it to the workflow. | ||
| # We defer this to later; the masks are set to None by default in the workflow for now. |
There was a problem hiding this comment.
Comment should be preserved.
…factor-widget-models
2 participants
Fixes #653
This is a codex rewrite of the parameter / workflow widget system using the models from esslivedata.