Add a trait for astropy quantities

[LukasBeiske]

Having a configurable trait containing an astropy.units.Quantity is useful for the irf tool and will probably be useful for other high-level tools, too.

[codecov]

Codecov Report

Attention: Patch coverage is 98.80952% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.62%. Comparing base (3952e5a) to head (c3046f5).
Report is 23 commits behind head on main.

Files	Patch %	Lines
src/ctapipe/core/traits.py	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2524      +/-   ##
==========================================
+ Coverage   92.53%   92.62%   +0.08%     
==========================================
  Files         235      233       -2     
  Lines       20063    20139      +76     
==========================================
+ Hits        18565    18653      +88     
+ Misses       1498     1486      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kosack]

Nice! One suggestion would be to add the ability to set a physical type or default unit, and have it fail if the user tries to set the value to a quantity that is not convertable.

I.e. if my trait is called energy_min and the user tries to set it to "12 m" it should fail.

That would mean either

1. adding a new field to the contructor for "physical_type"
2. just assuming if there is a default value, that any value should be convertable to the unit of that default and if no raise an error

The case where there is no unit check should still be handled, since there are use cases where you don't know the unit in advance.

[maxnoe]

Also missing are tests for setting quantities from the command line and in config files. For the CLI, you have to implement from_string I think, but probably just passing to u.Quantity is enough, maybe that's already the default.

[LukasBeiske]

Also missing are tests for setting quantities from the command line and in config files. For the CLI, you have to implement from_string I think, but probably just passing to u.Quantity is enough, maybe that's already the default.

I implemented from_string at first, but after testing it, it seems that everything works even without it. The test checks, if setting via a string works
Do you think additional test for config files and cmd are still needed? I think this is what happens in both cases, no?

[maxnoe]

Would be nice to make sure, can be as simple as:

class MyTool(Tool):
    energy = AstroQuantity(physical_type=u.physical.energy).tag(config=True)

tool = MyTool()
run_tool(tool, ["--MyTool.energy='5 TeV'"])
assert tool.energy == 5 * u.TeV

and maybe add a test that the error message is nice on invalid input

[ctao-dpps-sonarqube]

Analysis Details

4 Issues

• 0 Bugs
• 0 Vulnerabilities
• 4 Code Smells

Coverage and Duplications

• 98.80% Coverage (92.60% Estimated after merge)
• 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

[maxnoe]

@kosack I think this is ready, you requested changes so merger is currently blocked. Could you please check that your requested changes were implemented?