Overburden-to-height conversion functions #2422

gschwefer · 2023-10-25T10:15:07Z

Implements the overburden to height conversion functions in the atmosphere module

ctapipe/atmosphere.py

maxnoe · 2023-10-26T15:46:29Z

I wanted to make a simple plot of height vs. grammage using this function to check for the edge behavior for the cubic interpolation, but it flat out failed:

from ctapipe.io import EventSource
import numpy as np
import astropy.units as u

source = EventSource("dataset://gamma_prod5.simtel.zst")
grammage = np.linspace(0, 1200, 100) * u.g/u.cm**2
source.atmosphere_density_profile.height_from_overburden(grammage)

/home/maxnoe/Uni/CTA/ctapipe/ctapipe/atmosphere.py:307: RuntimeWarning: divide by zero encountered in log10
  self._height_interp(np.log10(overburden.to("g cm-2").value)), u.km
Traceback (most recent call last):
  File "/home/maxnoe/Uni/CTA/ctapipe/invalid_values.py", line 7, in <module>
    source.atmosphere_density_profile.height_from_overburden(grammage)
  File "/home/maxnoe/.local/conda/envs/cta-dev/lib/python3.11/site-packages/astropy/units/decorators.py", line 313, in wrapper
    return_ = wrapped_function(*func_args, **func_kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/atmosphere.py", line 307, in height_from_overburden
    self._height_interp(np.log10(overburden.to("g cm-2").value)), u.km
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/maxnoe/.local/conda/envs/cta-dev/lib/python3.11/site-packages/scipy/interpolate/_polyint.py", line 80, in __call__
    y = self._evaluate(x)
        ^^^^^^^^^^^^^^^^^
  File "/home/maxnoe/.local/conda/envs/cta-dev/lib/python3.11/site-packages/scipy/interpolate/_interpolate.py", line 755, in _evaluate
    below_bounds, above_bounds = self._check_bounds(x_new)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/maxnoe/.local/conda/envs/cta-dev/lib/python3.11/site-packages/scipy/interpolate/_interpolate.py", line 784, in _check_bounds
    raise ValueError("A value ({}) in x_new is below "
ValueError: A value (-inf) in x_new is below the interpolation range's minimum value (-4.864756005337933).

maxnoe · 2023-10-26T16:01:24Z

The error is mostly misleading, because it reports the transformed by log-values, so the user cannot really tell what is wrong easily

maxnoe · 2023-10-26T16:01:43Z

Please also rebase vs. current main to get the docs build working

ctapipe/tests/test_atmosphere.py

ctapipe/atmosphere.py

maxnoe · 2023-10-27T17:48:47Z

@gschwefer

I played around a bit locally and wanted to open a PR against your branch but accidentally pushed here directly.

Could you have a look at 51dbae7 and see if this ok with you? If not I'll revert

sorry

gschwefer · 2023-10-30T09:11:20Z

The commit looks alright. Should there be a test similar to the one you added for the other profiles as well, i.e. should we explicitly guard against inputing a negative height to the exponential model or is this a potentially relevant case?

maxnoe · 2023-10-30T09:16:15Z

@gschwefer Yes, I think the three classes should behave the same regarding invalid inputs, otherwise they are not interchangeable

ctapipe/atmosphere.py

kosack

Looks good, but we're still missing the inverse of AtmosphereProfile.line_of_site_integral where the zenith angle is taken into account (i.e. profile.distance_from_slant_depth(slant_depth, zenith_angle=60*u.deg) or whatever we want to call it), which is what you actually need for ImPACT, right?

gschwefer · 2023-11-02T08:00:37Z

ImPACT reconstructs slant depth, but what that is converted to as an output is up to whatever we decide it should. Currently, we would need a function that is height_from_slant_depth.

gschwefer · 2023-11-02T08:20:10Z

Another question about the functions currently there: Is the line_of_sight_integral function really correct? Because the distance you put into it is a distance from the observer, but the integral functions uses heights a.s.l.

maxnoe · 2023-11-02T15:47:54Z

It seems you ran an older version of black that formats the powers slightly differently.

Running black via pre-commit should use the same version as in the CI, e.g. copy the exact command of the CI:

$ pre-commit run --show-diff-on-failure --color=always --files $(git diff origin/main --name-only)

gschwefer · 2023-11-02T15:51:49Z

Yep thanks for the tip, I committed from a different ctapipe installation than normal where the pre-commit hooks are not setup properly

ctapipe/tests/test_atmosphere.py

maxnoe · 2024-02-23T09:40:04Z

Hi @gschwefer Could you fix the conflicts?

gschwefer · 2024-02-23T09:42:35Z

I'll have a look later today

codecov · 2024-02-23T14:19:05Z

Codecov Report

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

Project coverage is 92.66%. Comparing base (f8d1049) to head (0799687).
Report is 7 commits behind head on main.

Files	Patch %	Lines
src/ctapipe/atmosphere.py	97.22%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2422      +/-   ##
==========================================
+ Coverage   92.59%   92.66%   +0.06%     
==========================================
  Files         232      232              
  Lines       20034    20220     +186     
==========================================
+ Hits        18551    18736     +185     
- Misses       1483     1484       +1

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

maxnoe · 2024-02-23T14:21:18Z

docs build failure is unrelated, I am investigating

…polation

gschwefer · 2024-04-05T13:35:38Z

@maxnoe Looks like this tests fail is a github issue to me

maxnoe · 2024-04-05T13:41:00Z

Yes, seems to be something wrong / changed with the mac os base images, will check

maxnoe · 2024-04-05T13:44:16Z

I am a bit confused: https://github.com/actions/runner-images?tab=readme-ov-file#available-images

Says the macos-latest should be a macos 14 arm based image, but after I restarted the job, it says it's macos 12 on x86...

Maybe the rollout is not yet complete? And the failed run was running on arm / macos 14.

Edit: no, both were running on macos 12

maxnoe · 2024-04-05T14:11:45Z

Ok, was flaky, the latest run passed

maxnoe · 2024-04-05T14:18:45Z

Sonarqube is complaining about duplicated code in the tests, a large chunk is exactly equal:

    fit_reference = np.array(

        [

            [0.00 * 100000, -140.508, 1178.05, 994186, 0],

            [9.75 * 100000, -18.4377, 1265.08, 708915, 0],

            [19.00 * 100000, 0.217565, 1349.22, 636143, 0],

            [46.00 * 100000, -0.000201796, 703.745, 721128, 0],

            [106.00 * 100000, 0.000763128, 1, 1.57247e10, 0],

        ]

    )

    profile_5 = atmo.FiveLayerAtmosphereDensityProfile.from_array(fit_reference)

Could be a module constant or a fixture

gschwefer · 2024-04-05T14:29:19Z

I'll have a look

ctao-dpps-sonarqube · 2024-04-05T14:54:57Z

Analysis Details

2 Issues

0 Bugs
0 Vulnerabilities
2 Code Smells

Coverage and Duplications

98.60% Coverage (92.70% Estimated after merge)
0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

gschwefer requested review from kosack, maxnoe and Tobychev October 26, 2023 08:42

LukasNickel reviewed Oct 26, 2023

View reviewed changes

ctapipe/atmosphere.py Outdated Show resolved Hide resolved

Tobychev reviewed Oct 26, 2023

View reviewed changes

ctapipe/atmosphere.py Outdated Show resolved Hide resolved

Tobychev reviewed Oct 27, 2023

View reviewed changes

ctapipe/tests/test_atmosphere.py Outdated Show resolved Hide resolved

Tobychev previously approved these changes Oct 27, 2023

View reviewed changes

gschwefer dismissed Tobychev’s stale review via 2b6059c October 27, 2023 11:33

gschwefer force-pushed the grammage-to-altitude-conversion branch from c0c7f29 to 2b6059c Compare October 27, 2023 11:33

maxnoe reviewed Oct 27, 2023

View reviewed changes

ctapipe/atmosphere.py Outdated Show resolved Hide resolved

kosack reviewed Oct 31, 2023

View reviewed changes

ctapipe/atmosphere.py Outdated Show resolved Hide resolved

kosack requested changes Oct 31, 2023

View reviewed changes

Tobychev reviewed Nov 6, 2023

View reviewed changes

Tobychev previously approved these changes Nov 7, 2023

View reviewed changes

maxnoe mentioned this pull request Feb 23, 2024

Is the line_of_sight_integral function really correct? #2515

Closed

gschwefer dismissed Tobychev’s stale review via edffd2f February 23, 2024 14:06

maxnoe and others added 15 commits April 5, 2024 11:28

Improve unit handling in atmosphere module, use fill values for inter…

4a384fb

…polation

Edge cases and tests for exp and 5 layer profiles

804b74b

Implement function to get height from slant depth

7f3da52

ran black

6a19c11

Formtted again

a4fd39b

correct pre-commit hooks

99bf1ab

Include observer altitude in LoS integral

9c74c34

Test LoS integral height and zenith dependence

941c8eb

Test now using global units from atmosphere module

51f13be

Test value of LoS integral too

a950213

classes take Tables, work with QTables internally

3c2304b

Implement slant_depth_from_height function

1067eb0

remove superfluous dots

aa1fb9c

Rename test functions

4ced98a

Implement warning for very large zenith angles

5869a42

gschwefer force-pushed the grammage-to-altitude-conversion branch from 5b95eb2 to 5869a42 Compare April 5, 2024 09:29

gschwefer added 2 commits April 5, 2024 11:33

Include linter changes

8653377

Allow for array of zenith angles in 70 deg check

5cd88e5

This comment has been minimized.

Sign in to view

Implement 5 layer atmosphere as fixture

0799687

kosack approved these changes Apr 5, 2024

View reviewed changes

maxnoe approved these changes Apr 5, 2024

View reviewed changes

maxnoe merged commit e2a848e into cta-observatory:main Apr 5, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overburden-to-height conversion functions #2422

Overburden-to-height conversion functions #2422

gschwefer commented Oct 25, 2023

maxnoe commented Oct 26, 2023 •

edited

maxnoe commented Oct 26, 2023

maxnoe commented Oct 26, 2023

maxnoe commented Oct 27, 2023

gschwefer commented Oct 30, 2023

maxnoe commented Oct 30, 2023

kosack left a comment •

edited

gschwefer commented Nov 2, 2023

gschwefer commented Nov 2, 2023

maxnoe commented Nov 2, 2023

gschwefer commented Nov 2, 2023

maxnoe commented Feb 23, 2024

gschwefer commented Feb 23, 2024

codecov bot commented Feb 23, 2024 •

edited

maxnoe commented Feb 23, 2024

gschwefer commented Apr 5, 2024

maxnoe commented Apr 5, 2024

maxnoe commented Apr 5, 2024 •

edited

This comment has been minimized.

maxnoe commented Apr 5, 2024

maxnoe commented Apr 5, 2024

gschwefer commented Apr 5, 2024

ctao-dpps-sonarqube bot commented Apr 5, 2024

Overburden-to-height conversion functions #2422

Overburden-to-height conversion functions #2422

Conversation

gschwefer commented Oct 25, 2023

maxnoe commented Oct 26, 2023 • edited

maxnoe commented Oct 26, 2023

maxnoe commented Oct 26, 2023

maxnoe commented Oct 27, 2023

gschwefer commented Oct 30, 2023

maxnoe commented Oct 30, 2023

kosack left a comment • edited

Choose a reason for hiding this comment

gschwefer commented Nov 2, 2023

gschwefer commented Nov 2, 2023

maxnoe commented Nov 2, 2023

gschwefer commented Nov 2, 2023

maxnoe commented Feb 23, 2024

gschwefer commented Feb 23, 2024

codecov bot commented Feb 23, 2024 • edited

Codecov Report

maxnoe commented Feb 23, 2024

gschwefer commented Apr 5, 2024

maxnoe commented Apr 5, 2024

maxnoe commented Apr 5, 2024 • edited

This comment has been minimized.

maxnoe commented Apr 5, 2024

maxnoe commented Apr 5, 2024

gschwefer commented Apr 5, 2024

ctao-dpps-sonarqube bot commented Apr 5, 2024

Analysis Details

2 Issues

Coverage and Duplications

maxnoe commented Oct 26, 2023 •

edited

kosack left a comment •

edited

codecov bot commented Feb 23, 2024 •

edited

maxnoe commented Apr 5, 2024 •

edited