Polref data loader - #382
Conversation
Code for the polref legacy data loader and associated tests. Certain elements, such as logstep have been kept in since there is no direct numpy equivalent.
Added POLREF Ni58 script using the legacy loader, including data.
for more information, see https://pre-commit.ci
pkienzle
left a comment
There was a problem hiding this comment.
My comments are suggests. I'll defer to the polref team for the choice of interface.
| """ | ||
| Creates a log spaced 1d array by defining a step size and a base | ||
| In the form of dQ/Q - i.e. dQ\\Q*Qpoint | ||
| """ |
There was a problem hiding this comment.
Use: np.exp(np.arange(log(start), log(stop)+log(1+step), log(1+step)))
The result is independent of base.
|
|
||
|
|
||
| def TOF_loader(T=0.25, dQoQ=0.02, Q_sim_range=(0.005, 0.2), filename=None, skiprows=1, **kw): | ||
| """ |
There was a problem hiding this comment.
Suggest load_TOF (filename, T, dQoQ=0.02, ...).
load_TOF because function names are usually verbs (classes are usually nouns).
filename first because that corresponds to the order in the other loaders
T without default since you don't get theta offset and sample broadening otherwise. I'm assuming there is no good default value for theta.
dQoQ can default to 0.02 if that's what usually comes out of the reduction software. Specify in the docs that it is FWHM. If your usual Q spacing is set to dQoQ, then you can infer dQoQ from the Q spacing in the data.
Overloading the interface with filename=None to ask for a simulation is a little awkward, but I suppose it is okay.
Given that your wavelength range is relatively stable, I would expect the simulation to ask for an optional L_range=(low, high) that you can override if necessary instead of asking for Q_sim_range. [I'm assuming you are using theta control the instrument and not Qmin for each segment.]
If the L range is standard, then I guess you can infer theta from Q. That is, either T or L_range could be used as inputs. If reduction has a default L range in common use, then make it the default so you don't need to specify T or L_range when loading the file.
There was a problem hiding this comment.
@acaruana2009 Is there a significant existing set of models that use the existing signature? If so, I would argue that maybe keeping the less-than-ideal current signature makes sense, especially since this loader function is being added largely to support past and current models (with future models using the ORSO loader instead)
There was a problem hiding this comment.
Since the polref loader isn't in refl1d yet, existing models must be defining the reader in the script. None of these will find the new interface, so you are free to change it without breaking backward compatibility.
We will want a data loader as part of our model builder interface. This will probably look at file extension to choose the appropriate reader, or maybe it will cycle through all registered formats until it finds one that works. Without T and dQoQ, the polref reader can only return a Q probe without theta offset and sample broadening parameters.
We could provide a reader hook in webview which asks for this information but this will be hard to maintain. Instead have a separate tool that converts a directory from legacy format to ORSO and load the converted file into the model.
| return probe_out | ||
|
|
||
|
|
||
| def load_probe_polref(filename, angle, dQoQ, name=None, path=None, pol_mode=None, field=None, **kw): |
There was a problem hiding this comment.
Like load_TOF, you may not need angle or dQoQ.
| probe.intensity.tags = ["inst", "nuisance"] | ||
| probe.background.tags = ["inst", "nuisance"] | ||
| probe.sample_broadening.tags = ["inst", "nuisance"] | ||
| probe.theta_offset.tags = ["inst", "nuisance"] |
There was a problem hiding this comment.
These tags should be set in the Probe class.
There was a problem hiding this comment.
what does it mean, to have them set in the Probe class? Do you mean these should be the default tags for every Probe instance?
There was a problem hiding this comment.
These tags are not specific to polref data.
If we are going to set them in the loader, wouldn't it be best to do it generically so that they are consistent across loaders?
There was a problem hiding this comment.
Does that mean yes, you want to set them in the Probe init as the default tags?
There was a problem hiding this comment.
Yes, we can add tags=("inst", "nuisance") to all the Parameter.default initializers in probe.py. Here are the ones for Probe:
Lines 758 to 763 in 00f367e
I'm suggesting a tuple rather than a list because the input should be frozen. If code later tries to append to the list I want it to through an error rather than modify the default for the next call to Probe.
| probe.intensity.name = f"intensity {name}" | ||
| probe.background.name = f"background {name}" | ||
| probe.sample_broadening.name = f"sample_broadening {name}" | ||
| probe.theta_offset.name = f"theta_offset {name}" |
There was a problem hiding this comment.
These are the default names given within the Probe constructor, so no need to set them in the usual case. If you are passing them in as independent parameters, or parameters from another probe, you don't want to rename them here.
There was a problem hiding this comment.
See the latest commit. I have set the name in the correct place - this was probably left over from when I first wrote it years ago and wasn't aware of the name argument.
Removed the explicit setting of parameter names for probe parameters. Instead used the pre-existing `name` parameter in `Probe` and passed the `name` argument from `load_probe_polref` to `TOF_loader` and subsequently to `NeutronProbe`.
…data-loader # Conflicts: # refl1d/probe/data_loaders/polref_legacy_data_loader.py
for more information, see https://pre-commit.ci
| xs.intensity = probe.pp.intensity | ||
| xs.sample_broadening = probe.pp.sample_broadening | ||
| xs.theta_offset = probe.pp.theta_offset | ||
| xs.background = probe.pp.background |
There was a problem hiding this comment.
probe.shared_beam() does this.
There was a problem hiding this comment.
So I tried the below code, but the parameter name tags are lost - e.g. if name="ni58" that does not get passed to the parameter names. When looking at probe.shared_beam() the names are hard coded and have no scope to pull name from the probe class - or even accept an optional name from the call.
probe = PolarizedNeutronProbe(cross_sections, Aguide=270, H=field, name=name)
# looking at shared_beam, it doesn't set by default intensity etc.
# to the values originally passed when the probe object is created.
# Instead will pass them manually from probe.
probe.shared_beam(intensity=probe.pp.intensity.value,
background=probe.pp.background.value,
back_absorption=probe.pp.back_absorption.value,
theta_offset=probe.pp.theta_offset.value,
sample_broadening=probe.pp.sample_broadening.value)
If you would like me to use shared beam I would suggest changing probe.shared_beam() to something like:
def shared_beam(self, intensity=None, background=None, back_absorption=None, theta_offset=None, sample_broadening=None, name=None):
"""
Share beam parameters across all segments.
New parameters are created for *intensity*, *background*,
*theta_offset*, *sample_broadening* and *back_absorption*
and assigned to the all segments. These can be replaced
with an explicit parameter in an individual segment if that
parameter is independent.
"""
if not intensity:
intensity = self.probes[0].intensity
...
intensity = Parameter.default(intensity, name=f"intensity {name}")
background = Parameter.default(background, name=f"background {name}", limits=[0, None])
back_absorption = Parameter.default(back_absorption, name=f"back_absorption {name}", limits=[0, 1])
theta_offset = Parameter.default(theta_offset, name=f"theta_offset {name}")
sample_broadening = Parameter.default(sample_broadening, name=f"sample_broadening {name}", limits=[None, None])
for p in self.probes:
p.intensity = intensity
p.background = background
p.back_absorption = back_absorption
p.theta_offset = theta_offset
p.sample_broadening = sample_broadening
There was a problem hiding this comment.
How about adding the following to probe.py:
BEAM_TAGS = ('inst', 'nuisance')
SHARED_BEAM_KW = {'intensity', 'background', 'back_absorption', 'theta_offset', 'sample_broadening'}
...
def shared_beam(self, ...):
...
intensity = Parameter.default(intensity, name="intensity {self.name}", tags=BEAM_TAGS)
...
self.name defaults to mm.name if no name was given to PolarizedNeutronProbe.
Change PROBE_KW to create a set {...} instead of a tuple (...), and update every Parameter.default to include tags=BEAM_TAGS.
In load_probe_polref():
beam_kw = {k: v for k, v in kw.items() if k in SHARED_BEAM_KW}
probe_kw = {k: v for k, v in kw.items() if k not in SHARED_BEAM_KW}
...
for data in files.values():
TOF_loader(..., **probe_kw)
...
probe.shared_beam(**beam_kw)
In practice it seems that sample_broadening, back_absorption come from the sample and should always be shared.
Theta offset should be a property of alignment, so it should probably be shared. (If alignment changes with motion, then reflectivity curves are going to be hard to fit).
Intensity might be hard to normalize and background might depend on polarization state so I can imagine them being independent.
Maybe we could do partial sharing by calling with unshared parameters as None. For example, shared_beam(background=None) would share everything exception background.
| probe.pp.intensity.name = f"intensity {name}" | ||
| probe.pp.background.name = f"background {name}" | ||
| probe.pp.sample_broadening.name = f"sample_broadening {name}" | ||
| probe.pp.theta_offset.name = f"theta_offset {name}" |
There was a problem hiding this comment.
These parameters are already tagged with {name}.
If you switch to probe.shared_beam above it will create new parameters with untagged names. Please address #385 and remove the tagging here.
As requested by @christykinane, this is a stand alone data loader for 3-column POLREF data.
It includes helper functions such as
logstep, which mimics adQ/Qspacing for simulations by entering in adQ/Qresolution value - I took a look and I couldn't see anumpyfunction that precisely replicated what we have here, I am happy to replace as appropriate.I have included tests (written by Claude code), example data and an example model.
Tests seem to be running fine on my machine.