Workflow dependency restructure #155
Conversation
docs/user-guide/isis/sans2d.ipynb
Outdated
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "workflow[BeamCenter] = sc.vector([0, 0, 0], unit='m')\n", |
There was a problem hiding this comment.
I find it strange from a UX point of view that we first need to set the center to [0,0,0] before computing it...
But I guess you made it so because we don't want to have it set to zero by default in the workflow because users will forget to compute it?
There was a problem hiding this comment.
Yes. It could in principle also be used to set a default that then gets refined... and I think I should add a test to ensure that we still get the absolute (relative to [0,0,0]) beam center back in a such as case.
There was a problem hiding this comment.
I think this is in fact not working: If we set anything but [0,0,0] the beam-center finder(s) return a beam center relative to the previously set center, which is not what we want, I think?
There was a problem hiding this comment.
Please see update, as discussed on the call.
| sample_offset: SampleOffset, | ||
| detector_bank_offset: DetectorBankOffset, |
There was a problem hiding this comment.
I keep changing my mind about these things. Are they just ISIS specifics that will go away at ESS and therefore should just be corrected in the input files and not appear here at all, or are they going to be very common and done for almost every experiment, and thus should be part of the general SANS workflow...
There was a problem hiding this comment.
Time will tell? We can refactor later?
| @@ -22,15 +22,19 @@ | |||
| """Sample holder mask""" | |||
|
|
|||
|
|
|||
| def detector_edge_mask(sample: TofData[SampleRun]) -> DetectorEdgeMask: | |||
| # It may make more sense to depend on CalibratedDetector here, but the current | |||
There was a problem hiding this comment.
Yes, that would make more sense, but we need the masks to be there before we start computing a center of mass, which is annoying.
There was a problem hiding this comment.
The masks are anyway applied after setting the beam center (see your other comment #155 (comment)). So this could easily be changed, but maybe that is not how the scientists work? Do you remember where these numbers come from?
There was a problem hiding this comment.
Note than in principle we could set masks before loading data (when they come from a file).... but maybe that is not desirable if other types of masks come into play?
There was a problem hiding this comment.
Numbers for masks came from trial and error with Wojciech. We just wanted to mask the edges and found values that worked.
| pipeline = sciline.Pipeline(providers, params=params) | ||
| pipeline[MaskedData[SampleRun]] = data | ||
| calibrated = pipeline.compute(CalibratedMaskedData[SampleRun]) | ||
| graph = workflow.compute(ElasticCoordTransformGraph) |
There was a problem hiding this comment.
Nice you could remove all of the above 👍
Yes, failing due to unreleased and unreviewed changes in ESSreduce. |
SimonHeybrock
force-pushed
the
split-event-load
branch
from
eb0cd45
to
dc72e50
Compare
SimonHeybrock
force-pushed
the
workflow-dependency-restructure
branch
from
98126e4
to
00eb05d
Compare
Fixes #154.
There are some subtleties here, around different way of handling ESS NeXus vs. Mantid files.
The I(Q) beam-center finder was a headache, but I hope by passing the workflow I managed to make it a bit more natural. There is potential room for improvement, by avoiding reloading data even if positions change, but I left it for now until we know if this actually is an issue in practice.