Unify methods in common module, implement engines1d in FiberIntegrator by EdgarGF93 · Pull Request #2809 · silx-kit/pyFAI

EdgarGF93 · 2026-03-03T14:03:21Z

Engines1d

EdgarGF93 · 2026-03-03T14:54:36Z

Some benchmark:

`Timing: ('no', 'histogram', 'python'), through 2d engine
569 ms ± 934 μs per loop (mean ± std. dev. of 7 runs, 1 loop each)
Timing: ('no', 'histogram', 'python'), through 1d engine
80.9 ms ± 278 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)


Timing: ('no', 'histogram', 'cython'), through 2d engine
93 ms ± 214 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)
Timing: ('no', 'histogram', 'cython'), through 1d engine
80.8 ms ± 349 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)

Copilot

Pull request overview

This PR refactors fiber/grazing-incidence integration to unify 1D/2D method handling and introduce an “engines1d” path in FiberIntegrator, while also centralizing mask-building logic in the common integrator base.

Changes:

Added FiberIntegrator._integrate_fiber to share logic between integrate1d_fiber/integrate2d_fiber and to support use_2d_engine=False (direct engine path).
Introduced create_mask_generic in integrator/common.py and updated AzimuthalIntegrator.integrate1d to use it for azimuth-range masking.
Added preferred default fiber methods in load_engines.py and a new equivalence test for 2D-engine vs 1D-engine fiber integration.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/pyFAI/test/test_fiber_integrator.py	Adds a regression/equivalence test for `use_2d_engine` vs engine path.
src/pyFAI/integrator/load_engines.py	Adds preferred default integration methods for fiber 1D/2D.
src/pyFAI/integrator/fiber.py	Major refactor: unify 1D/2D fiber integration + add engine-based implementation path.
src/pyFAI/integrator/common.py	Adds `create_mask_generic` and refactors `create_mask` to delegate to it.
src/pyFAI/integrator/azimuthal.py	Uses `create_mask_generic` for azimuth-range masking in histogram 1D path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T17:01:54Z

+            result_fiber._set_std(result_tuple.std)
+            result_fiber._set_sem(result_tuple.sem)
+            result_fiber._set_sum_normalization2(result_tuple.norm_sq)
+            result_fiber._set_compute_engine = f"{method.class_funct_ng.function.__module__}:{method.class_funct_ng.function.__name__}"


_set_compute_engine is a setter method on result objects, but here it’s being overwritten with a string (result_fiber._set_compute_engine = ...). This will lose the method and leave compute_engine unset. Call result_fiber._set_compute_engine(...) instead.

Suggested change

result_fiber._set_compute_engine = f"{method.class_funct_ng.function.__module__}:{method.class_funct_ng.function.__name__}"

result_fiber._set_compute_engine(f"{method.class_funct_ng.function.__module__}:{method.class_funct_ng.function.__name__}")

Copilot · 2026-03-23T17:01:55Z

+                    params_integrator.update({
+                        "mask" : mask,
+                        "radial" : self.center_array(shape, unit=projected_unit, scale=False),
+                        "radial_range" : projected_range,
+                        "npt" : projected_bins,
+                    })
+                elif method.dim == 2:
+                    # For 2d, there's no need to mask before the integration
+                    mask = self.create_mask_generic(
+                        data=data,
+                        mask=mask,
+                    )     
+                    params_integrator.update({
+                        "mask" : mask,
+                        "radial" : self.center_array(shape, unit=unit_ip, scale=False),
+                        "radial_range" : ip_range,
+                        "azimuthal" : self.array_from_unit(shape, "center", unit_oop, scale=False),
+                        "azimuth_range" : oop_range,
+                        "bins" : (npt_ip, npt_oop),
+                        "dark_variance" : None,
+                        "allow_radial_neg" : True,
+                    })


In the engine-based path (use_2d_engine=False), radial is computed with scale=False, but radial_range/azimuth_range are passed through unnormalized. Other integrators normalize ranges into internal coordinates by dividing by unit.scale before comparing/using them. Without doing the same here, any non-trivial unit scale (e.g. degrees vs radians) will lead to incorrect masking/bin selection when ranges are provided.

Copilot · 2026-03-23T17:01:55Z

+    if "sum_normalization" in dir(result):
+        intensity = result.sum_normalization
+    elif "normalization" in dir(result):
+        intensity = result.normalization


get_missing_wedge_mask can return an UnboundLocalError if the passed result object has neither sum_normalization nor normalization attributes (because intensity would be undefined). Using hasattr and raising a clear TypeError/AttributeError in the fallback case would make failures deterministic and easier to debug.

Suggested change

if "sum_normalization" in dir(result):

intensity = result.sum_normalization

elif "normalization" in dir(result):

intensity = result.normalization

if hasattr(result, "sum_normalization"):

intensity = result.sum_normalization

elif hasattr(result, "normalization"):

intensity = result.normalization

else:

raise AttributeError(

"result object must define either 'sum_normalization' or 'normalization' "

"attributes to compute the missing wedge mask"

)

Copilot · 2026-03-23T17:01:55Z

+            res_engine = self.fi.integrate_fiber(use_2d_engine=False, method=method, **params_gi)
+            self.assertTrue(numpy.allclose(res_from_2d.intensity, res_engine.intensity))


This equivalence test only compares intensity. Given the refactor changes how the output axis is constructed/scaled and how units/metadata are set, it would be good to also assert that the returned coordinate array (integrated) matches between the two paths (and sigma too when available). Otherwise regressions in axis scaling/unit handling can slip through while intensities still match.

Suggested change

res_engine = self.fi.integrate_fiber(use_2d_engine=False, method=method, **params_gi)

self.assertTrue(numpy.allclose(res_from_2d.intensity, res_engine.intensity))

res_engine = self.fi.integrate_fiber(use_2d_engine=False, method=method, **params_gi)

self.assertTrue(numpy.allclose(res_from_2d.radial, res_engine.radial))

self.assertTrue(numpy.allclose(res_from_2d.intensity, res_engine.intensity))

if (

hasattr(res_from_2d, "sigma")

and hasattr(res_engine, "sigma")

and (res_from_2d.sigma is not None)

and (res_engine.sigma is not None)

):

self.assertTrue(numpy.allclose(res_from_2d.sigma, res_engine.sigma))

Copilot · 2026-03-23T17:01:56Z

+            unit1_array = self.array_from_unit(shape, "center", unit1)
+            logical_or(mask, unit1_array < unit1_range[0], out=mask)
+            logical_or(mask, unit1_array > unit1_range[1], out=mask)
+        if unit2_range is not None:
+            if unit2 is None:
+                raise RuntimeError("integrated_unit is needed when building a mask based on integrated_unit_range")
+            unit2_array = self.array_from_unit(shape, "center", unit2)


In create_mask_generic, array_from_unit is called with the default scale=True, while the previous create_mask logic compared against internal (unscaled) arrays (scale=False). This will make unit*_range masking inconsistent with callers that already normalized ranges by dividing by unit.scale (e.g. AzimuthalIntegrator.integrate1d). Consider passing scale=False here (and ensuring ranges are in the same internal scale), to preserve behavior and avoid masking the wrong pixels.

Suggested change

unit1_array = self.array_from_unit(shape, "center", unit1)

logical_or(mask, unit1_array < unit1_range[0], out=mask)

logical_or(mask, unit1_array > unit1_range[1], out=mask)

if unit2_range is not None:

if unit2 is None:

raise RuntimeError("integrated_unit is needed when building a mask based on integrated_unit_range")

unit2_array = self.array_from_unit(shape, "center", unit2)

unit1_array = self.array_from_unit(shape, "center", unit1, scale=False)

logical_or(mask, unit1_array < unit1_range[0], out=mask)

logical_or(mask, unit1_array > unit1_range[1], out=mask)

if unit2_range is not None:

if unit2 is None:

raise RuntimeError("integrated_unit is needed when building a mask based on integrated_unit_range")

unit2_array = self.array_from_unit(shape, "center", unit2, scale=False)

Copilot · 2026-03-23T17:01:56Z

+            result_tuple = Integrate2dtpl(
+                res2d_fiber.radial,
+                res2d_fiber.azimuthal,
+                res2d_fiber.intensity,
+                res2d_fiber.sem,


Integrate2dtpl expects sigma as the 4th field, but this is currently populated with res2d_fiber.sem. This swaps standard error/SEM into the sigma slot and can break downstream code that assumes result_tuple.sigma is the propagated error. Use res2d_fiber.sigma (or None if not available) for that field.

Suggested change

result_tuple = Integrate2dtpl(

res2d_fiber.radial,

res2d_fiber.azimuthal,

res2d_fiber.intensity,

res2d_fiber.sem,

sigma = getattr(res2d_fiber, "sigma", None)

result_tuple = Integrate2dtpl(

res2d_fiber.radial,

res2d_fiber.azimuthal,

res2d_fiber.intensity,

sigma,

Copilot · 2026-03-23T17:01:56Z

+                                    radial_range=ip_range,
+                                    azimuth_range=oop_range,
+                                    unit=(unit_ip, unit_oop),
+                                    filename=None)


When use_2d_engine=True, the call to integrate2d_ng ignores the public variance/error_model arguments passed into integrate1d_fiber/integrate2d_fiber. As a result, res2d_fiber.sigma/sum_variance/std/sem will never be computed in this path, and the API behaves differently depending on use_2d_engine. Pass variance=... and error_model=... through to integrate2d_ng (and consider also propagating metadata).

Suggested change

filename=None)

filename=None,

variance=variance,

error_model=error_model,

metadata=metadata)

Copilot · 2026-03-23T17:01:57Z

+            if method.dim == 1:
+                result_fiber = Integrate1dFiberResult(
+                    intensity=result_tuple.intensity,
+                        integrated=result_tuple.position * integrated_unit.scale,
+                        sigma=result_tuple.sigma,
+                )
+                result_fiber._set_vertical_integration(vertical_integration)
+                result_fiber._set_unit(projected_unit)
+            elif method.dim == 2:


The 1D fiber result is built with integrated=result_tuple.position * integrated_unit.scale, but the position vector produced above is the projected axis (oop when vertical_integration=True, ip otherwise). This both applies the wrong unit’s scale factor and can double-scale the axis when coming from integrate2d_ng (which already returns scaled bin centers). The output Integrate1dFiberResult.integrated values and result_fiber._set_unit(...) can end up inconsistent; use the projected axis and its scale consistently (or avoid extra scaling when the 2D engine already returns scaled positions).

EdgarGF93 added the work in progress Don't review label Mar 16, 2026

Copilot AI review requested due to automatic review settings March 23, 2026 16:54

Copilot started reviewing on behalf of EdgarGF93 March 23, 2026 16:55 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

edgar1993a added 17 commits March 24, 2026 10:02

merge integrated unit to mask method

add628c

use merge integrated for azimuthal integrator

105da50

implement engine for method=no-histogram-python/cython

1376631

add test for equivalent integrations

f3aa4e8

method create_mask_generic

a0a74b0

docs for create_mask_generic

cc9ecf5

use create_mask_generic

983ea52

fix create_mask

3132e12

prefered methods fiber

9cae231

mask generic

e6f2700

ai mask generic

7adecd8

refactor fiber integrator

4e57876

change defaults in fiber

5de7414

fix tests

9404624

simplify no numexpr

392ba0d

fix _set_compute_engine

4db05c1

do not use create_mask

8e1bc54

EdgarGF93 force-pushed the move_integrator_methods branch from 8ceb1b1 to 8e1bc54 Compare March 24, 2026 09:02

common back to original

784028a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify methods in common module, implement engines1d in FiberIntegrator#2809

Unify methods in common module, implement engines1d in FiberIntegrator#2809
EdgarGF93 wants to merge 18 commits into
silx-kit:mainfrom
EdgarGF93:move_integrator_methods

EdgarGF93 commented Mar 3, 2026 •

edited

Loading

Uh oh!

EdgarGF93 commented Mar 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	result_fiber._set_compute_engine = f"{method.class_funct_ng.function.__module__}:{method.class_funct_ng.function.__name__}"
	result_fiber._set_compute_engine(f"{method.class_funct_ng.function.__module__}:{method.class_funct_ng.function.__name__}")

		res_engine = self.fi.integrate_fiber(use_2d_engine=False, method=method, **params_gi)
		self.assertTrue(numpy.allclose(res_from_2d.intensity, res_engine.intensity))

-            res_engine = self.fi.integrate_fiber(use_2d_engine=False, method=method, **params_gi)
-            self.assertTrue(numpy.allclose(res_from_2d.intensity, res_engine.intensity))
+            res_engine = self.fi.integrate_fiber(use_2d_engine=False, method=method, **params_gi)
+            self.assertTrue(numpy.allclose(res_from_2d.radial, res_engine.radial))
+            self.assertTrue(numpy.allclose(res_from_2d.intensity, res_engine.intensity))
+            if (
+                hasattr(res_from_2d, "sigma")
+                and hasattr(res_engine, "sigma")
+                and (res_from_2d.sigma is not None)
+                and (res_engine.sigma is not None)
+            ):
+                self.assertTrue(numpy.allclose(res_from_2d.sigma, res_engine.sigma))

-                                    filename=None)
+                                    filename=None,
+                                    variance=variance,
+                                    error_model=error_model,
+                                    metadata=metadata)

Conversation

EdgarGF93 commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EdgarGF93 commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EdgarGF93 commented Mar 3, 2026 •

edited

Loading

EdgarGF93 commented Mar 3, 2026 •

edited

Loading