From b3dd24f41dc67700c38c76f862d71f8bfe39b5a2 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Tue, 20 Jan 2026 16:54:33 +0100 Subject: [PATCH 01/10] Fix FeatureGenerator gives wrong units When a `Table` class is passed to `FeatureGeneator`, instead of a `QTable`, the units are not correctly propagated. E.g., if a source column `x` has units `m`, a generated column `x**2` should have units `m2`, but does not if the input table is of class Table, where it retains the incorrect unit `m`. Unit propagation works for QTables. --- src/ctapipe/core/feature_generator.py | 4 ++- .../core/tests/test_feature_generator.py | 28 ++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/ctapipe/core/feature_generator.py b/src/ctapipe/core/feature_generator.py index 4d330b8fedc..b0b96850612 100644 --- a/src/ctapipe/core/feature_generator.py +++ b/src/ctapipe/core/feature_generator.py @@ -4,6 +4,8 @@ from collections import ChainMap +from astropy.table import QTable, Table + from .component import Component from .expression_engine import ExpressionEngine from .traits import List, Tuple, Unicode @@ -54,7 +56,7 @@ def __init__(self, config=None, parent=None, **kwargs): self.engine = ExpressionEngine(expressions=self.features) self._feature_names = [name for name, _ in self.features] - def __call__(self, table, **kwargs): + def __call__(self, table: Table | QTable, **kwargs) -> QTable: """ Apply feature generation to the input table. diff --git a/src/ctapipe/core/tests/test_feature_generator.py b/src/ctapipe/core/tests/test_feature_generator.py index 3ba3fb0d153..75b0d1b1011 100644 --- a/src/ctapipe/core/tests/test_feature_generator.py +++ b/src/ctapipe/core/tests/test_feature_generator.py @@ -2,7 +2,7 @@ import numpy as np import pytest -from astropy.table import Table +from astropy.table import QTable, Table from ctapipe.core.expression_engine import ExpressionError from ctapipe.core.feature_generator import FeatureGenerator, FeatureGeneratorException @@ -68,6 +68,7 @@ def test_to_unit(): table = generator(table) assert table["length_meter"] == 1000 assert table["log_length_meter"] == 3 + assert table["length_meter"].unit == u.m def test_multiplicity(subarray_prod5_paranal): @@ -102,3 +103,28 @@ def test_multiplicity(subarray_prod5_paranal): np.testing.assert_equal(table["n_lsts"], [1, 2]) np.testing.assert_equal(table["n_msts"], [2, 1]) np.testing.assert_equal(table["n_ssts"], [0, 1]) + + +@pytest.mark.parametrize("table_class", [QTable, Table]) +def test_unit_propegation(table_class): + """ + Check that units propagate to features. + + If a column in the input table has a unit, and a feature does math on that + unit, the feature should have the appropriate unit. + """ + + import astropy.units as u + + table = table_class(dict(x=np.arange(11) * u.cm, E=np.linspace(-2, 2, 11) * u.TeV)) + features = [ + ("x2", "x**2"), + ("E_per_area", "E/x**2"), + ] + + feature_gen = FeatureGenerator(features=features) + + new_table = feature_gen(table) + + assert new_table["x2"].unit.is_equivalent("cm2") + assert new_table["E_per_area"].unit.is_equivalent("TeV cm-2") From b0b12892d5d0c72bc2a7be8160c81aeedabf9f9f Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Tue, 20 Jan 2026 17:31:42 +0100 Subject: [PATCH 02/10] added changelog --- docs/changes/2921.api.rst | 7 +++++++ docs/changes/2921.bugfix.rst | 4 ++++ 2 files changed, 11 insertions(+) create mode 100644 docs/changes/2921.api.rst create mode 100644 docs/changes/2921.bugfix.rst diff --git a/docs/changes/2921.api.rst b/docs/changes/2921.api.rst new file mode 100644 index 00000000000..e9be2f25386 --- /dev/null +++ b/docs/changes/2921.api.rst @@ -0,0 +1,7 @@ +As a consequence of fixing the bug #2921, `ctapipe.core.ExpressionEngine` +converts all input tables to `astropy.table.QTable` internally, which has a +small side effect on what is allowed in expressions: all columns with units are +now of type `astropy.units.Quantity`, instead of `astropy.table.Column`. Before, +an expression like ``"some_column.quantity.to(u.m)"`` would work if a ``Table`` +was passed (but would fail for a ``QTable``). Now, that expression should be +``some_column.to(u.m)`` diff --git a/docs/changes/2921.bugfix.rst b/docs/changes/2921.bugfix.rst new file mode 100644 index 00000000000..22ca32ae837 --- /dev/null +++ b/docs/changes/2921.bugfix.rst @@ -0,0 +1,4 @@ +Fixed bug where units were incorrect in the output table of an +`ctapipe.core.ExpressionEngine` if a table of class `astropy.table.Table` was +passed to the call method. This bug did not affect calls using an +`astropy.table.QTable`. From 65a5db1f8750e5ee3cc1e3c058cb30a77fe19510 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Tue, 20 Jan 2026 17:32:26 +0100 Subject: [PATCH 03/10] implement fix, modify test, and add better docs --- src/ctapipe/core/feature_generator.py | 23 ++++++++++++++++--- .../core/tests/test_feature_generator.py | 4 ++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/ctapipe/core/feature_generator.py b/src/ctapipe/core/feature_generator.py index b0b96850612..37fda3e0ed4 100644 --- a/src/ctapipe/core/feature_generator.py +++ b/src/ctapipe/core/feature_generator.py @@ -4,7 +4,7 @@ from collections import ChainMap -from astropy.table import QTable, Table +from astropy.table import QTable from .component import Component from .expression_engine import ExpressionEngine @@ -56,15 +56,32 @@ def __init__(self, config=None, parent=None, **kwargs): self.engine = ExpressionEngine(expressions=self.features) self._feature_names = [name for name, _ in self.features] - def __call__(self, table: Table | QTable, **kwargs) -> QTable: + def __call__(self, table: QTable, **kwargs) -> QTable: """ Apply feature generation to the input table. This method returns a shallow copy of the input table with the new features added. Existing columns will share the underlying data, however the new columns won't be visible in the input table. + + Parameters + ---------- + table: QTable | Table + Input table. Internally a Table will be converted to a QTable so that + unit propagation works, so expressions should only rely on properties of QTables. + **kwargs: + Other objects that should be available in expressions. For example, + if a you pass ``subarray=subarray``, expressions can use that + object. This can also be special functions like `f=my_function`, + which would allow an expression like "f(col1)". + + Returns + ------- + QTable: + A new table with the same columns as the input, but with new columns + for each feature. """ - table = _shallow_copy_table(table) + table = _shallow_copy_table(QTable(table)) lookup = ChainMap(table, kwargs) for result, name in zip(self.engine(lookup), self._feature_names): diff --git a/src/ctapipe/core/tests/test_feature_generator.py b/src/ctapipe/core/tests/test_feature_generator.py index 75b0d1b1011..0f5b97dd69b 100644 --- a/src/ctapipe/core/tests/test_feature_generator.py +++ b/src/ctapipe/core/tests/test_feature_generator.py @@ -60,13 +60,13 @@ def test_to_unit(): expressions = [ ("length_meter", "length.to(u.m)"), - ("log_length_meter", "log10(length.quantity.to_value(u.m))"), + ("log_length_meter", "log10(length.to_value(u.m))"), ] generator = FeatureGenerator(features=expressions) table = Table({"length": [1 * u.km]}) table = generator(table) - assert table["length_meter"] == 1000 + assert table["length_meter"] == 1000 * u.m assert table["log_length_meter"] == 3 assert table["length_meter"].unit == u.m From d4567ff27c84c4df0d744f8afa320af8759c3747 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Tue, 20 Jan 2026 17:44:21 +0100 Subject: [PATCH 04/10] ensure return value is the same as input --- src/ctapipe/core/feature_generator.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/ctapipe/core/feature_generator.py b/src/ctapipe/core/feature_generator.py index 37fda3e0ed4..74f0f6a2400 100644 --- a/src/ctapipe/core/feature_generator.py +++ b/src/ctapipe/core/feature_generator.py @@ -4,7 +4,7 @@ from collections import ChainMap -from astropy.table import QTable +from astropy.table import QTable, Table from .component import Component from .expression_engine import ExpressionEngine @@ -56,7 +56,7 @@ def __init__(self, config=None, parent=None, **kwargs): self.engine = ExpressionEngine(expressions=self.features) self._feature_names = [name for name, _ in self.features] - def __call__(self, table: QTable, **kwargs) -> QTable: + def __call__(self, table: Table | QTable, **kwargs) -> Table: """ Apply feature generation to the input table. @@ -77,22 +77,22 @@ def __call__(self, table: QTable, **kwargs) -> QTable: Returns ------- - QTable: + QTable|Table: A new table with the same columns as the input, but with new columns - for each feature. + for each feature. The returned class depends on what was passed in. """ - table = _shallow_copy_table(QTable(table)) - lookup = ChainMap(table, kwargs) + table_copy = _shallow_copy_table(QTable(table)) + lookup = ChainMap(table_copy, kwargs) for result, name in zip(self.engine(lookup), self._feature_names): - if name in table.colnames: + if name in table_copy.colnames: raise FeatureGeneratorException(f"{name} is already a column of table.") try: - table[name] = result + table_copy[name] = result except Exception as err: raise err - return table + return table.__class__(table_copy) # ensure the return type is what is expected def __len__(self): return len(self.features) From 5373a621f5e4d771758b55a5c946a6d98a0ce36a Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Tue, 20 Jan 2026 17:49:42 +0100 Subject: [PATCH 05/10] add a test for return value of __call__() --- .../core/tests/test_feature_generator.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/ctapipe/core/tests/test_feature_generator.py b/src/ctapipe/core/tests/test_feature_generator.py index 0f5b97dd69b..b29509d5551 100644 --- a/src/ctapipe/core/tests/test_feature_generator.py +++ b/src/ctapipe/core/tests/test_feature_generator.py @@ -123,8 +123,25 @@ def test_unit_propegation(table_class): ] feature_gen = FeatureGenerator(features=features) - new_table = feature_gen(table) assert new_table["x2"].unit.is_equivalent("cm2") assert new_table["E_per_area"].unit.is_equivalent("TeV cm-2") + + +@pytest.mark.parametrize("table_class", [QTable, Table]) +def test_input_output_class(table_class): + """Ensure output table class is same as input.""" + + import astropy.units as u + + table = table_class(dict(x=np.arange(11) * u.cm, E=np.linspace(-2, 2, 11) * u.TeV)) + features = [ + ("x2", "x**2"), + ("E_per_area", "E/x**2"), + ] + + feature_gen = FeatureGenerator(features=features) + new_table = feature_gen(table) + + assert new_table.__class__ == table.__class__ From 92b1f60edc1e3ad706ea825e1c383153794c69e9 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Tue, 20 Jan 2026 21:23:25 +0100 Subject: [PATCH 06/10] fix formatting in docstring --- src/ctapipe/core/feature_generator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ctapipe/core/feature_generator.py b/src/ctapipe/core/feature_generator.py index 74f0f6a2400..747bdb36e4f 100644 --- a/src/ctapipe/core/feature_generator.py +++ b/src/ctapipe/core/feature_generator.py @@ -72,8 +72,8 @@ def __call__(self, table: Table | QTable, **kwargs) -> Table: **kwargs: Other objects that should be available in expressions. For example, if a you pass ``subarray=subarray``, expressions can use that - object. This can also be special functions like `f=my_function`, - which would allow an expression like "f(col1)". + object. This can also be special functions like ``f=my_function``, + which would allow an expression like ``"f(col1)"``. Returns ------- From 789f17026f47812ebd562e4771231b917ad12e94 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 21 Jan 2026 09:52:42 +0100 Subject: [PATCH 07/10] fix link in changelog --- docs/changes/2921.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changes/2921.bugfix.rst b/docs/changes/2921.bugfix.rst index 22ca32ae837..a8b0e8c1d27 100644 --- a/docs/changes/2921.bugfix.rst +++ b/docs/changes/2921.bugfix.rst @@ -1,4 +1,4 @@ Fixed bug where units were incorrect in the output table of an -`ctapipe.core.ExpressionEngine` if a table of class `astropy.table.Table` was +`ctapipe.core.FeatureGenerator` if a table of class `astropy.table.Table` was passed to the call method. This bug did not affect calls using an `astropy.table.QTable`. From fa4a09aea9e9681740e38d6a0f2291bccbbe491a Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 28 Jan 2026 10:05:05 +0100 Subject: [PATCH 08/10] explose core.ExpressionEngine, for docs --- src/ctapipe/core/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ctapipe/core/__init__.py b/src/ctapipe/core/__init__.py index f83471c864f..f5d7dffb4c3 100644 --- a/src/ctapipe/core/__init__.py +++ b/src/ctapipe/core/__init__.py @@ -5,6 +5,7 @@ from .component import Component, non_abstract_children from .container import Container, DeprecatedField, Field, FieldValidationError, Map +from .expression_engine import ExpressionEngine from .feature_generator import FeatureGenerator from .provenance import Provenance, get_module_version from .qualityquery import QualityCriteriaError, QualityQuery @@ -28,4 +29,5 @@ "QualityQuery", "QualityCriteriaError", "FieldValidationError", + "ExpressionEngine", ] From aef58852e01d5e052c941600ac17987dbaeebd19 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 28 Jan 2026 11:05:00 +0100 Subject: [PATCH 09/10] type conversion and metadata in shallow_copy_table --- src/ctapipe/core/feature_generator.py | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/ctapipe/core/feature_generator.py b/src/ctapipe/core/feature_generator.py index 747bdb36e4f..fbb1f02fe81 100644 --- a/src/ctapipe/core/feature_generator.py +++ b/src/ctapipe/core/feature_generator.py @@ -3,6 +3,7 @@ """ from collections import ChainMap +from copy import deepcopy from astropy.table import QTable, Table @@ -13,19 +14,31 @@ __all__ = [ "FeatureGenerator", "FeatureGeneratorException", + "shallow_copy_table", ] -def _shallow_copy_table(table): +def shallow_copy_table( + table, output_cls: type[Table] | type[QTable] | None = None +) -> Table | QTable: """ Make a shallow copy of the table. - Data of the existing columns will be shared between shallow - copies, but adding / removing columns won't be seen in - the original table. + Data of the existing columns will be shared between shallow copies, but + adding / removing columns won't be seen in the original table. Metadata for + the new table will be a copy (not shallow) of the original metadata, so that + new metadata can be added without affecting the original table. + + Parameters + ---------- + output_cls: type[Table] | type[QTable] | None + type of the output table. If None, use the input table type """ - # automatically return Table or QTable depending on input - return table.__class__({col: table[col] for col in table.colnames}, copy=False) + output_cls = output_cls or table.__class__ + + new_table = output_cls({col: table[col] for col in table.colnames}, copy=False) + new_table.meta = deepcopy(table.meta) + return new_table class FeatureGeneratorException(TypeError): @@ -81,7 +94,7 @@ def __call__(self, table: Table | QTable, **kwargs) -> Table: A new table with the same columns as the input, but with new columns for each feature. The returned class depends on what was passed in. """ - table_copy = _shallow_copy_table(QTable(table)) + table_copy = shallow_copy_table(table, output_cls=QTable) lookup = ChainMap(table_copy, kwargs) for result, name in zip(self.engine(lookup), self._feature_names): From 5a5642233124a9cd4b0cd77510d36e8cac5a57cd Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 28 Jan 2026 13:55:40 +0100 Subject: [PATCH 10/10] Update src/ctapipe/core/tests/test_feature_generator.py Co-authored-by: Maximilian Linhoff --- src/ctapipe/core/tests/test_feature_generator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctapipe/core/tests/test_feature_generator.py b/src/ctapipe/core/tests/test_feature_generator.py index b29509d5551..65f88c0adcd 100644 --- a/src/ctapipe/core/tests/test_feature_generator.py +++ b/src/ctapipe/core/tests/test_feature_generator.py @@ -106,7 +106,7 @@ def test_multiplicity(subarray_prod5_paranal): @pytest.mark.parametrize("table_class", [QTable, Table]) -def test_unit_propegation(table_class): +def test_unit_propagation(table_class): """ Check that units propagate to features.