scipp / esssans

SANS data reduction for the European Spallation Source
https://scipp.github.io/esssans/
BSD 3-Clause "New" or "Revised" License
0 stars 2 forks source link

Avoid having both `QBins` and `QxyBins` in the task graph, as they are mutually exclusive #98

Closed nvaytet closed 7 months ago

nvaytet commented 9 months ago

Following guideline in https://github.com/scipp/ess/issues/219.

Instead, we could maybe make a single parameter? However, it may not be as user-friendly?

Currently:

# for 1d Q bins
params[QBins] = sc.geomspace(dim='Q', start=0.004, stop=0.8, num=141, unit='1/angstrom')

# for Qxy bins
params[QxyBins] = {
    'Qx': sc.linspace(dim='Qx', start=-0.5, stop=0.5, num=101, unit='1/angstrom'),
    'Qy': sc.linspace(dim='Qy', start=-0.8, stop=0.8, num=101, unit='1/angstrom'),
}

Could become:

# for 1d Q bins
params[QBins] = {'Q': sc.geomspace(dim='Q', start=0.004, stop=0.8, num=141, unit='1/angstrom')}

# for Qxy bins
params[QBins] = {
    'Qx': sc.linspace(dim='Qx', start=-0.5, stop=0.5, num=101, unit='1/angstrom'),
    'Qy': sc.linspace(dim='Qy', start=-0.8, stop=0.8, num=101, unit='1/angstrom'),
}

but maybe annoying to have to define a dict if we are only doing 1d Q binning?

Another suggestion was:

# for 1d Q bins
params[QBins] = [sc.geomspace(dim='Q', start=0.004, stop=0.8, num=141, unit='1/angstrom')]

# for Qxy bins
params[QBins] = [
    sc.linspace(dim='Qx', start=-0.5, stop=0.5, num=101, unit='1/angstrom'),
    sc.linspace(dim='Qy', start=-0.8, stop=0.8, num=101, unit='1/angstrom'),
]

and just infer from the dims?

SimonHeybrock commented 9 months ago

I think I had briefly tried something like this, but then we need to check or decide what to do when a dict/list with all there coords is provided. Or maybe that is what we should do, and additionally have a flag the defines which one to use?

nvaytet commented 9 months ago

I guess if someone supplies all 3 you may get 3 dimensions in the output? Q, Qx, Qy. It would be weird but consistent?

SimonHeybrock commented 9 months ago

That makes little sense.

jl-wynen commented 9 months ago

We don't support unions as params, right? So we could so something like

# for 1d Q bins
params[QBins] = QBins.q(sc.geomspace(dim='Q', start=0.004, stop=0.8, num=141, unit='1/angstrom'))

# for Qxy bins
params[QBins] = QBins.qxy(
    sc.linspace(dim='Qx', start=-0.5, stop=0.5, num=101, unit='1/angstrom'),
    sc.linspace(dim='Qy', start=-0.8, stop=0.8, num=101, unit='1/angstrom'),
)

Trying to find a way to make invalid states unrepresentable.

SimonHeybrock commented 7 months ago

Picking this up since the removal of Optional support in Sciline will require changes around this anyway.