Avoid use of `Optional`

[SimonHeybrock]

With the recent (unreleased) change of how Sciline handled Optional, we need to find other solutions. This is an attempt to do this, while staying compatible to the released Sciline.

Cases:

Use default workflow param

• There are some simple cases where Optional is not really needed
• Example: Instead of Optional[MonitorOffset], use MonitorOffset and define a default of [0,0,0] m
• Example: Instead of Optional[CorrectForGravity] (i.e., Optional[bool], define default of False.

Good switch to using a class instead of an alias

Main examples is #98, where I removed QxyBins and added a QBins class, which does both. The users needs to wrap the variable in QBins, which is a bit annoying. Biggest concern is that this is not raised early, due to scipp/sciline#160, so maybe this needs to be reconsidered?

Awkward switch to using a class instead of an alias

I am "faking" an optional by having a wrapping dataclass in many cases. I do not like this, since the caller needs to be aware, and we need to access the properties of those classes in several places, including in computed intermediate results (like DirectBeam).

Better ideas wanted!

More notes

Risk of bugs

I ran into a concerning case with both the old and new behavior: We previously implicitly (via depending on Optional[DirectBeam] pruned the DirectBeam branch of DirectBeamFilename was not set. However, the DirectBeam is also set manually in other cases. This will then silently ignore the DirectBeamFilename. This is particularly concerning since one might be tempted (in other words: I did this during refactor) to set a default of params[DirectBeam] = DirectBeam().

My thought is that with the Sciline rewrite we may avoid this by composing the pipeline via __setitem__, using something like

# Option 1: pipeline from file (currently: set DirectBeamFilename)
direct_beam = sans.load_direct_beam_pipeline(db_file)
# Option 2: from a in-memory direct beam (currently: set DirectBeam)
direct_beam = compute_direct_beam_pipeline.compute(DirectBeam)
# Option 3: no direct beam (currently set neither)
direct_beam = DirectBeam()
pipeline[DirectBeam] = direct_beam

Time will tell, I hope?

Is Sciline/ADR0001 (remove isinstance checks) a bad idea?

When adding those wrappers, the isinstance checks from the current Sciline told me where I forgot to use the wrapper. In the unreleased Sciline we get a more confusing compute-time error.

To do

• Some things in the ISIS/Mantid case not working yet.
• Beam center finder notebook

[SimonHeybrock]

Some suggestions, to improve at least some parts:

• Change Optional[WavelengthMask] not to a wrapping class but redefine it as a dict[str, sc.Variable]. Having named masks may be better anyway, and the user could specify multiple (or we can use map/reduce, once available).
• Consider moving the content of the provider for ProcessedWavelengthBands into the constructor of WavelengthBands. Apart from the special case of not setting bands (then they are defined via the WavelengthBins) I think this looks cleaner? Not sure how best to handle this fallback case though.

[SimonHeybrock]

I'm overall not too happy with how this impacts both usability and implementation.

Yes, my point exactly. But would you be happy with it if we ignore backward-compat, and just use Optional[Param] as keys? Or do you still have concerns then?

Handling default params explicitly is fine. I think the problem only comes in when there are alternatives to a parameter (optionals or variants like QBins). A user has to know which parameters need to be wrapped and which don't. The same goes for implementers of providers.

For the optionals, that is why the next Sciline will let you do pipeline[Optional[Param]] = param. For the QBins I do not have an answer, but in that particular case I don't think there is really a way around this, and you suggested something similar in https://github.com/scipp/esssans/issues/98#issuecomment-1965976198.

This is just a suggestion and we should talk about it in person: How about all parameters must be wrapped in a special class? Sciline could provide a base class to help. Then there would be no confusion about whether parameters need to be wrapped/unwrapped or not. When constructing a pipeline, all param values that are not wrapped get automatically wrapped by their key type, e.g., this

Not sure what exactly one would gain with such a wrapper? Basically, you suggest to add something like

self._params = param if isinstance(sciline.Param) else key(param)

in Pipeline.__getitem__? That seems worse than a simple isinstance check (prior to ADR 0001). Wouldn't we get a similar benefit with a (non-broken) version of our old isinstance check?

params = {
  Period: 3,
  NonBackgroundWavelengthRange = NonBackgroundWavelengthRange(...)
}

becomes

params = {
  Period: Period(3),
  NonBackgroundWavelengthRange = NonBackgroundWavelengthRange(...)
}

The param type could even, for non-variants, inherit from the underlying type so that consumers can use it directly like the underlying type.

[nvaytet]

I have a feeling we may need to also consider https://github.com/scipp/essreduce/issues/21 for some of the discussions here?

[jl-wynen]

Not sure what exactly one would gain with such a wrapper? Basically, you suggest to add something like

self._params = param if isinstance(sciline.Param) else key(param)

in Pipeline.getitem? That seems worse than a simple isinstance check (prior to ADR 0001). Wouldn't we get a similar benefit with a (non-broken) version of our old isinstance check?

I don't understand. Why would this be worse? All params would be classes. SO there would be no issues with type aliases.

Essentially, the suggestion is about having a uniform interface. All parameters can be defined as

params = {A: A(2)}

regardless of whether it is strictly needed to wrap the value or not. And the same goes for consumers. A plain parameter is always accessed as a.value. Variants need special properties and maybe constructors.

The suggestion about auto wrapping and deriving from the the underlying param type are about avoiding the explicit wrapping and unwrapping but are less relevant in my opinion.

[SimonHeybrock]

This will be replaced.