Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/pypi-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ jobs:

- name: Run integration tests to verify the installation
working-directory: easyreflectometry
run: pixi run python -m pytest ../tests/integration/ --color=yes -n auto
# No -n auto: concurrent xdist workers race on arviz's daily-warning
# stamp file when they import easyreflectometry. See pixi.toml.
run: pixi run python -m pytest ../tests/integration/ --color=yes

# Job 2: Build and publish dashboard (reusable workflow)
run-reusable-workflows:
Expand Down
4 changes: 3 additions & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,9 @@ jobs:
cd easyreflectometry_py$py_ver

echo "Running tests"
pixi run python -m pytest ../tests/integration/ --color=yes -n auto -v ${{ needs.env-prepare.outputs.pytest-marks }}
# No -n auto: concurrent xdist workers race on arviz's daily-warning
# stamp file when they import easyreflectometry. See pixi.toml.
pixi run python -m pytest ../tests/integration/ --color=yes -v ${{ needs.env-prepare.outputs.pytest-marks }}

echo "Exiting pixi project directory"
cd ..
Expand Down
31 changes: 31 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,36 @@
# Unreleased

Fixed inconsistent interpretation of vector resolution functions between
the refnx and refl1d engines (issue #367).

- **Reflectivity results change for two engine / resolution
combinations.** `LinearSpline` on refl1d previously **over-smeared by
a factor of 2.355** (its FWHM widths were passed to refl1d's
`probe.dQ`, which expects sigma). `Pointwise` on refnx previously
**under-smeared by the same factor** (its sigma widths were passed to
refnx's `x_err`, which expects FWHM). Both are now correct. Fits and
simulations that used either combination will produce different —
previously wrong — results and should be re-run. `PercentageFwhm` on
either engine, `LinearSpline` on refnx, and `Pointwise` on refl1d are
numerically unchanged.
- `ResolutionFunction.smearing()` now returns **sigma** (the Gaussian
standard deviation) for every subclass; each engine wrapper converts
to its backend's convention. This is a behavioural change to a public
method. Most visibly, `PercentageFwhm.smearing(q)` used to return the
_percentage_ itself (e.g. `5.0`) and now returns an absolute sigma
(e.g. `0.00212` at `q=0.1`); `LinearSpline.smearing(q)` returns its
`fwhm_values` divided by `2*sqrt(2*ln2)`. Callers relying on the old
values need to convert. The new `SIGMA_TO_FWHM` constant is exported
from `easyreflectometry.model.resolution_functions`.
- Constructors are **unchanged**: `PercentageFwhm(5)` still means 5%
FWHM and `LinearSpline(q, fwhm_values)` still takes FWHM. Only the
`smearing()` output convention moved, so existing model-building code
needs no edits.
- `PercentageFwhm.smearing(q)` given a scalar `q` now returns a 0-d
numpy scalar rather than a shape-`(1,)` array, matching
`LinearSpline`. `smearing(0.1)[0]` therefore raises `IndexError` where
it previously returned a value.

Migrated sample / model classes off the deprecated `easyscience.ObjBase`
and `easyscience.CollectionBase` pipeline.

Expand Down
3 changes: 1 addition & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ Please make sure you follow the EasyScience organization-wide
If you are not planning to contribute code, you may want to:

- 🐞 Report a bug — see [Reporting Issues](#11-reporting-issues)
- 🛡 Report a security issue — see
[Security Issues](#12-security-issues)
- 🛡 Report a security issue — see [Security Issues](#12-security-issues)
- 💬 Ask a question or start a discussion at
[Project Discussions](https://github.com/easyscience/reflectometry-lib/discussions)

Expand Down
3 changes: 2 additions & 1 deletion docs/mkdocs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ nav:
- Elements:
- Layers:
- Layer: api-reference/elements/layer.md
- Layer Area Per Molecule: api-reference/elements/layer_area_per_molecule.md
- Layer Area Per Molecule:
api-reference/elements/layer_area_per_molecule.md
- Materials:
- Material: api-reference/elements/material.md
- Material Density: api-reference/elements/material_density.md
Expand Down
1 change: 1 addition & 0 deletions pixi.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion pixi.toml
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,15 @@ user = { features = ['py-max', 'user'] }

unit-tests = 'python -m pytest tests/unit/ --color=yes -v'
functional-tests = 'python -m pytest tests/functional/ --color=yes -v'
integration-tests = 'python -m pytest tests/integration/ --color=yes -n auto -v'
# No -n auto: importing easyreflectometry pulls in arviz, and arviz 0.23.4
# (py-311-env) writes a "warn once per day" stamp file on import via a
# _atomic_write_text() that is not atomic -- every process writes the same
# fixed `daily_warning.tmp` and renames it onto `daily_warning`. Concurrent
# xdist workers therefore race on that rename: FileNotFoundError on Linux,
# PermissionError (WinError 32) on Windows. The real fix is
# to stop importing arviz in easyreflectometry/__init__.py, after which
# xdist can come back.
integration-tests = 'python -m pytest tests/integration/ --color=yes -v'
notebook-tests = 'python -m pytest --nbmake docs/docs/tutorials/**/ --nbmake-timeout=1200 --color=yes -n auto -v'

test = { depends-on = ['unit-tests'] }
Expand Down
7 changes: 1 addition & 6 deletions src/easyreflectometry/calculators/refl1d/wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
from refl1d import names
from refl1d.sample.layers import Repeat

from easyreflectometry.model import PercentageFwhm

from ..wrapper_base import WrapperBase

RESOLUTION_PADDING = 3.5
Expand Down Expand Up @@ -205,12 +203,9 @@ def calculate(self, q_array: np.ndarray, model_name: str) -> np.ndarray:
Reflectivity calculated at q.
"""
sample = _build_sample(self.storage, model_name)
# smearing() returns sigma, which is exactly what refl1d's probe.dQ expects.
dq_array = self._resolution_function.smearing(q_array)

if isinstance(self._resolution_function, PercentageFwhm):
# Get percentage of Q and change from sigma to FWHM
dq_array = dq_array * q_array / 100 / (2 * np.sqrt(2 * np.log(2)))

if not self._magnetism:
probe = _get_probe(
q_array=q_array,
Expand Down
10 changes: 7 additions & 3 deletions src/easyreflectometry/calculators/refnx/wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from refnx import reflect

from easyreflectometry.model import PercentageFwhm
from easyreflectometry.model.resolution_functions import SIGMA_TO_FWHM

from ..wrapper_base import WrapperBase

Expand Down Expand Up @@ -191,9 +192,12 @@ def calculate(self, q_array: np.ndarray, model_name: str) -> np.ndarray:

dq_vector = self._resolution_function.smearing(q_array)
if isinstance(self._resolution_function, PercentageFwhm):
# FWHM Percentage resolution is constant given as
# For a constant resolution percentage refnx supports to pass a scalar value rather than a vector
dq_vector = dq_vector[0]
# refnx interprets a scalar x_err as a constant dq/q (FWHM percentage),
# so pass the percentage directly rather than a per-point vector.
dq_vector = self._resolution_function.constant
else:
# smearing() returns sigma; refnx expects the FWHM at each point.
dq_vector = dq_vector * SIGMA_TO_FWHM

return model(x=q_array, x_err=dq_vector)

Expand Down
42 changes: 34 additions & 8 deletions src/easyreflectometry/model/resolution_functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@
Gaussian distribution with a FWHM of the percentage of the q value.
To convert from a sigma value to a FWHM value we use the formula
FWHM = 2.35 * sigma [2 * np.sqrt(2 * np.log(2)) * sigma].

The :meth:`ResolutionFunction.smearing` contract returns **sigma**
(the standard deviation of the Gaussian resolution) for every resolution
type. This matches the ``sQz`` convention used by data reduction and the
natural output of :class:`Pointwise`. Each calculation engine wrapper is
responsible for converting sigma to the width convention of its backend
(FWHM for refnx, sigma for refl1d), so that vector resolutions are
interpreted consistently across engines (see GitHub issue #367).
"""

from __future__ import annotations
Expand All @@ -19,10 +27,15 @@

DEFAULT_RESOLUTION_FWHM_PERCENTAGE = 5.0

# Conversion factor between sigma and FWHM for a Gaussian: FWHM = SIGMA_TO_FWHM * sigma.
SIGMA_TO_FWHM = 2 * np.sqrt(2 * np.log(2))

Check warning on line 31 in src/easyreflectometry/model/resolution_functions.py

View check run for this annotation

Codecov / codecov/patch

src/easyreflectometry/model/resolution_functions.py#L31

Added line #L31 was not covered by tests


class ResolutionFunction:
@abstractmethod
def smearing(self, q: Union[np.array, float]) -> np.array: ...
def smearing(self, q: Union[np.array, float]) -> np.array:

Check warning on line 36 in src/easyreflectometry/model/resolution_functions.py

View check run for this annotation

Codecov / codecov/patch

src/easyreflectometry/model/resolution_functions.py#L36

Added line #L36 was not covered by tests
"""Return the resolution as sigma (standard deviation) at each ``q``."""
...

@abstractmethod
def as_dict(self, skip: Optional[List[str]] = None) -> dict: ...
Expand Down Expand Up @@ -51,8 +64,14 @@
self.constant = constant

def smearing(self, q: Union[np.array, float]) -> np.array:
"""Smearing function."""
return np.ones(np.array(q).size) * self.constant
"""Return per-point sigma values from the constant FWHM percentage.

``constant`` is a FWHM percentage of ``q``; it is converted to an
absolute sigma so the smearing() contract is sigma for all types.
"""
q_array = np.asarray(q, dtype=float)
fwhm = (self.constant / 100.0) * q_array
return fwhm / SIGMA_TO_FWHM

Check warning on line 74 in src/easyreflectometry/model/resolution_functions.py

View check run for this annotation

Codecov / codecov/patch

src/easyreflectometry/model/resolution_functions.py#L72-L74

Added lines #L72 - L74 were not covered by tests

def as_dict(
self, skip: Optional[List[str]] = None
Expand All @@ -68,8 +87,13 @@
self.fwhm_values = fwhm_values

def smearing(self, q: Union[np.array, float]) -> np.array:
"""Smearing function."""
return np.interp(q, self.q_data_points, self.fwhm_values)
"""Return per-point sigma values from the FWHM knots.

The stored ``fwhm_values`` are FWHM widths; they are interpolated
onto ``q`` and converted to sigma to satisfy the smearing() contract.
"""
fwhm = np.interp(np.asarray(q, dtype=float), self.q_data_points, self.fwhm_values)
return fwhm / SIGMA_TO_FWHM

Check warning on line 96 in src/easyreflectometry/model/resolution_functions.py

View check run for this annotation

Codecov / codecov/patch

src/easyreflectometry/model/resolution_functions.py#L95-L96

Added lines #L95 - L96 were not covered by tests

def as_dict(
self, skip: Optional[List[str]] = None
Expand Down Expand Up @@ -110,10 +134,12 @@
self.q_data_points = q_data_points

def smearing(self, q: Optional[Union[np.ndarray, float]] = None) -> np.ndarray:
"""Return the resolution width interpolated onto ``q``.
"""Return the resolution sigma interpolated onto ``q``.

The width at each data point is ``sqrt(sQz)``; values are linearly
interpolated onto the requested ``q``. When ``q`` is ``None`` the widths
``sQz`` is the variance of ``Qz``, so the sigma at each data point is
``sqrt(sQz)``; values are linearly interpolated onto the requested
``q``. This already satisfies the sigma smearing() contract, so no
FWHM conversion is applied. When ``q`` is ``None`` the sigma values
are returned at the stored data points.
"""
Qz = np.asarray(self.q_data_points[0], dtype=float)
Expand Down
2 changes: 1 addition & 1 deletion tests/calculators/refnx/test_refnx_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ def test_calculate_github_test4_spline_resolution(self):
p.add_item('Item3', 'MyModel')
p.add_item('Item4', 'MyModel')
p.update_model('MyModel', bkg=0)
sigma_to_fwhm = 2.355
sigma_to_fwhm = 2.0 * np.sqrt(2.0 * np.log(2.0))
p.set_resolution_function(LinearSpline(test4_dat[:, 0], sigma_to_fwhm * test4_dat[:, 3]))
assert_allclose(p.calculate(test4_dat[:, 0], 'MyModel'), test4_dat[:, 1], rtol=0.03)

Expand Down
Loading
Loading