Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chunk interpolation to select calibration data #2634

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Conversation

ctoennis
Copy link

I will need a method to select calibration data for the strar tracker. I made some slides to decribe how it is supposed to work:
https://docs.google.com/presentation/d/1oxIcYSQvGnU7IQYy3fGdcv0qXiLpvaXR9YtmnDesj4Y/edit?usp=sharing

@ctoennis ctoennis requested review from maxnoe and kosack October 31, 2024 11:27
@ctoennis ctoennis self-assigned this Oct 31, 2024

This comment has been minimized.

3 similar comments

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@ctoennis
Copy link
Author

@maxnoe @kosack Can you have another look if there is something else to be changed?

@mexanick
Copy link
Contributor

@kosack @maxnoe this PR is needed to complete the pointing calibration (for the variance calibration application), can we advance it?

@ctoennis
Copy link
Author

I am a bit stuck here with one of the tests. test_hdf5 is failing in pytest begause the data from the file is not loaded correctly. When i look in the test what x and y values the interpolators have i get some wrong values. However if i try to do the same outside of pytest it works. I used this code to test by myself:

import astropy.units as u
import numpy as np
import tables
from astropy.table import Table
from astropy.time import Time

from functools import partial
from ctapipe.core import Component, traits

from ctapipe.monitoring.interpolation import PointingInterpolator

from ctapipe.io import write_table

t0 = Time("2022-01-01T00:00:00")

table = Table(
    {"time": t0 + np.arange(0.0, 10.1, 2.0) * u.s, "azimuth": np.radians(np.linspace(0.0, 10.0, 6)) * u.rad, "altitude": np.radians(>)

path = "pointing.h5"

write_table(table, path, "/dl0/monitoring/telescope/pointing/tel_001")

with tables.open_file(path) as h5file:
    interpolator = PointingInterpolator(h5file)
    t = t0 + 1 * u.s
    alt, az = interpolator(tel_id=1, time=t)
    print(interpolator._interpolators[1]["alt"].y,interpolator._interpolators[1]["alt"].x)
    print(alt,az)

Has anyone an idea what is wrong here?

@mexanick
Copy link
Contributor

@ctoennis the problem you're experiencing with the test is because you have class attributes in the base class that are shared between the class instances, including the _interpolators.

@ctoennis
Copy link
Author

@mexanick Thanks, i got the issue fixed.

@ctoennis ctoennis requested review from maxnoe and mexanick November 26, 2024 08:42
@ctoennis
Copy link
Author

@maxnoe Hi Max, could you give this another look?

src/ctapipe/monitoring/interpolation.py Outdated Show resolved Hide resolved
Simple interpolator for overlapping chunks of data.
"""

required_columns = frozenset(["start_time", "end_time"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an immutable type that you "mutate" further down in the code. Please don't do this, if you want to extend it with the value column, particular for an object, just make a copy and to the object attribute and modify that one.

"""

required_columns = frozenset(["start_time", "end_time"])
expected_units = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected units can be different for different objects. Please ensure you can create two different ChunkInterpolators objects with different interpolation column and different units to it.

super().__init__(**kwargs)

self._interpolators = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed before, the base class should be rather pure. An interface shouldn't prescribe private data layout.

This comment has been minimized.

1 similar comment
Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 96.30% Coverage (94.10% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants