Skip to content

Fix FeatureGenerator gives wrong units#2921

Merged
kosack merged 10 commits into
mainfrom
feature_gen_units
Jan 28, 2026
Merged

Fix FeatureGenerator gives wrong units#2921
kosack merged 10 commits into
mainfrom
feature_gen_units

Conversation

@kosack
Copy link
Copy Markdown
Member

@kosack kosack commented Jan 20, 2026

FeatureGeneartors currently support both Table and QTable as inputs, and both are tested. However, unit propagation is wrong when aTable with units is used.

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. See the test I added to this PR, which currently fails.

I found this when looking into a simpler implementation for #2919 .

Possible solutions:

  • require QTables in ExpressionEngine, and raise an exception if a Table is passed.
  • Convert all Tables to QTables in __call__() (breaks current tests where a feature column calls col.quantity.unit)

The reason is that Astropy does not do unit math for columns automatically (which I think is actually quite a strange choice)

>>> table = Table(dict(x=np.arange(11)*u.cm)
>>> table["x"]**2
<Column name='x' dtype='float64' unit='cm' length=11>
  0.0
  1.0
  4.0
  9.0
 16.0
 25.0
 36.0
 49.0
 64.0
 81.0
100.0

@kosack
Copy link
Copy Markdown
Member Author

kosack commented Jan 20, 2026

Do we rely anywhere on using FeatureGenerator on Tables with unit columns? If not, I can modify the tests and change the API to explicilty require QTables. Supporting both is somewhat annoying.

EDIT: I decided to simply change to using internal conversion to QTable, which has the side effect of not allowing one expression we previously tested: "log10(length.quantity.to_value(u.m))" now must be "log10(length.to_value(u.m))"

@kosack kosack requested a review from maxnoe January 20, 2026 16:34
@kosack kosack force-pushed the feature_gen_units branch 2 times, most recently from e78ee74 to f0b9120 Compare January 21, 2026 09:02
@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Jan 21, 2026

We should just add QTable(table, copy=False) to the call function I think. That's a very cheap operation.

Comment thread src/ctapipe/core/feature_generator.py Outdated
@kosack
Copy link
Copy Markdown
Member Author

kosack commented Jan 22, 2026

Once #2923 is merged, the docs here should pass.

@kosack kosack force-pushed the feature_gen_units branch from f0b9120 to 535e8a7 Compare January 22, 2026 16:58
@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Jan 26, 2026

Docs build is fixed in main, please rebase

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.
@kosack kosack force-pushed the feature_gen_units branch from 535e8a7 to 789f170 Compare January 26, 2026 17:35
Comment thread src/ctapipe/core/tests/test_feature_generator.py Outdated
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@cta-observatory.org>
@maxnoe maxnoe requested a review from mexanick January 28, 2026 12:58
@ctao-sonarqube
Copy link
Copy Markdown

@kosack kosack merged commit b2dc9c2 into main Jan 28, 2026
21 of 22 checks passed
@kosack kosack deleted the feature_gen_units branch January 28, 2026 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants