cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
62 stars 266 forks source link

Add a trait for astropy quantities #2524

Closed LukasBeiske closed 3 months ago

LukasBeiske commented 3 months ago

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[bot] commented 3 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

maxnoe commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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[bot] commented 3 months ago

Passed

Analysis Details

4 Issues

Coverage and Duplications

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

maxnoe commented 3 months ago

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