Conversation
…ly to WavelengthDetector from RawDetector
| @@ -0,0 +1,187 @@ | |||
| # SPDX-License-Identifier: BSD-3-Clause | |||
There was a problem hiding this comment.
I moved the stuff about larmor into a separate file, to leave the loki workflow cleaner.
| source_position=source_position, | ||
| gravity=gravity, | ||
| ) | ||
| return ElasticCoordTransformGraph[RunType]({**graph, **tof.elastic_Q('tof')}) |
There was a problem hiding this comment.
Note that this is new in the workflow submodule (was not in the old workflow.py file. We put the functions from the tof graph back in, as a tof was recorded in the files at Larmor (short pulse source).
…r used by the generic sans workflow
There was a problem hiding this comment.
Updated the reference solutions: the data values and variances have remained identical. However, some coordinates pertaining to positions (L1, source_position, sample_position...) are no longer present in the final result. We have Q and wavelength, and this is probably the way it should be.
If we want to add contributions from different measurements, we should no longer care about these positions once we are in Q-space.
| direct_beam: CleanDirectBeam, | ||
| uncertainties: UncertaintyBroadcastMode, | ||
| ) -> WavelengthDetector[RunType, Denominator]: | ||
| ) -> DetectorTerm[RunType, Denominator]: |
There was a problem hiding this comment.
So in summary we have:
- Old:
RawDetector->TofDetector->CorrectedDetector->WavelengthDetector - New:
RawDetector->WavelengthDetector->CorrectedDetector->DetectorTerm
That is:
- Corrected and Wavelength swapped... which brings it in line with https://scipp.github.io/essreduce/user-guide/reduction-workflow-guidelines.html#c-6-domain-types-for-data-products
- New intermediate
DetectorTerm
| SANS workflow as a sciline.Pipeline | ||
| """ | ||
| workflow = GenericTofWorkflow( | ||
| workflow = GenericUnwrapWorkflow( |
There was a problem hiding this comment.
To be honest, I would have just kept the GenericTofWorkflow - we are still at a TOF neutron source after all.
There was a problem hiding this comment.
I don't really mind, others also suggested other names.
But this is quite late at this point. Would have been easier to pick a different name earlier.
I'm thinking maybe we don't need the "Generic" in there if we don't want the name to become too long.
So it could become something like ComputeWavelengthWorkflow? Or Just WavelengthWorkflow.
I could change the name, and then keep GenericUnwrapWorkflow for a while as an alias. And phase it out later once we've updated the other packages, so that we don't break everything again.
There was a problem hiding this comment.
I don't really care.
we are still at a TOF neutron source after all.
Does this mean we also need LokiTofWorkflow and OdinTofWorkflow? I think it's enough that the class is defined in an 'ess' package.
There was a problem hiding this comment.
I would keep LokiWorkflow and OdinWorkflow, as they will always want to compute wavelength.
There was a problem hiding this comment.
In any case, the naming cannot be changed in this PR, it needs to be done first in essreduce.
| LookupTableRelativeErrorThreshold = unwrap_t.LookupTableRelativeErrorThreshold | ||
| LookupTableFilename = unwrap_t.LookupTableFilename | ||
| LookupTable = unwrap_t.LookupTable |
There was a problem hiding this comment.
Seems a bit odd to import these unqualified "LookupTable" types directly into the top-level (types module).
There was a problem hiding this comment.
So you would just import unwrap and then have
from ess.sans.types import unwrap
unwrap.LookupTable?
I can do that.
There was a problem hiding this comment.
But maybe that is confusing because our visualize() currently strips module names from type names.
3 participants
Needed after scipp/ess#65
See some comments below that walk a little through the more confusing changes.