Interpolator #2600
Interpolator #2600
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
Why do you put everything into one big constructor instead of having a base class and with several more specialised children? Is there some specific technical reason for why you want to entirely eliminate the |
|
@Tobychev Yes, i should split it up like that. I wanted to have something working first, but i'll restructure it. |
This comment has been minimized.
This comment has been minimized.
|
@ctoennis I think we need separate classes here, per thing that can be interpolated, putting common code into free function or a common base class. This has to do with our configuration system: it configures things by class name. So to be able to have two interpolators with different configurations (e.g. for input file from which it reads the monitoring table), we need different class names. |
|
@maxnoe im doing that right now |
|
I split it up into a parent and 2 child classes. I am now updating the docustrings. |
This comment has been minimized.
This comment has been minimized.
src/ctapipe/io/interpolating.py
Outdated
| return self.values[i - 1] | ||
|
|
||
|
|
||
| class Interpolator(Component): |
There was a problem hiding this comment.
This should probably be an abstract base class
There was a problem hiding this comment.
which means adding metaclass=ABCMeta as second argument here.
src/ctapipe/io/interpolating.py
Outdated
|
|
||
| def add_table(self, tel_id, input_table): | ||
| # sort first, so it's not done twice for each interpolator | ||
| input_table.sort("time") |
There was a problem hiding this comment.
This doesn't do anything here. Remove all code from this method and make it an @abstractmethod
src/ctapipe/io/interpolating.py
Outdated
| mjd, alt, **self.interp_options | ||
| ) | ||
|
|
||
| def _read_parameter_table(self, tel_id): |
There was a problem hiding this comment.
This could probably be put into the base class, if we add a class attribute table_path like this:
class PointingInterpolator(Interpolator):
table_path = "/dl0/monitoring/telescope/pointing"
class PedestalInterpolator(Interpolator):
table_path = "/dl0/monitoring/telescope/pedestal"
src/ctapipe/io/interpolating.py
Outdated
| interpolated calibration quantity | ||
| """ | ||
|
|
||
| if tel_id not in self._interpolators: |
There was a problem hiding this comment.
This is probably the same for all subclasses, so it could be put in the base class.
This comment has been minimized.
This comment has been minimized.
|
So, i split the Calibration Interpolator into a pedestal and gain interpolator. This way the variable par_name is not needed anymore. |
|
sorry for th oversight with the class variable. I moved it to the definition and made it so the other calibration data is required to have a dimensionless unit. |
src/ctapipe/io/interpolation.py
Outdated
| @@ -11,6 +11,8 @@ | |||
|
|
|||
| from .astropy_helpers import read_table | |||
|
|
|||
| dimensionless_unit = u.Unit() | |||
There was a problem hiding this comment.
There e is no need to define this.
There is u.one as short alias or u.dimensionless_unscaled if you want to be explicit.
There was a problem hiding this comment.
It seems that the Flat field interpolator is not clear / final. It has some TBD comments, but most importantly, it doesn't match the definition of the dl1 calibration container.
I think this should match, right? The interpolator fills the values to be used for the calibration.
So, the pedestal and flatfield calibration data will in the end have different time values and the interpolator will have to load each type of data from a different table. To me it makes sense to therefore have a different interpolator for those fields in the DL1CameraCalibrationContainer. The TBD comments relate to the location where the input groups for the different types of the calibration data will be stored. This would be settled when we add the CalibrationCalculators. Some of these, like the PointingCalculator will need to have the interpolator there to interpolate gain or pedestal data for their calculations. I think it would make sense to leave these fields with the TBD comments and fix this when we implement the CalibrationCalculators. |
|
I think it would make more sense then to only introduce the base class in this PR here and change the pointing interpolator. The other interpolators can be added once the TBDs are clear. It doesn't make much sense to have interpolators in main for which no input data exists. |
|
i will move the other interpolators into the branch me and tjark have for the CalibrationCalculators then and remove them here. Does that make sense? |
|
@maxnoe can you have look again? i think it should be good to merge now. |
This PR is for extending the pointing interpolator to also handle pedestal and gain. This request is related to issue #2565.
A new interpolator is added based on the searchsorted function in numpy and set up to accept the same options as interp1d class from numpy that is already in use for the pointing interpolation. This interpolator is creating a step function that applies a pedestal or gain calibration after a corresponding timestamp until the next pedestal or gain value is available.
The new interpolator is able to interpolate array-like objects to handle
It is currently set up to automatically recognise what data is interpolated and picks an appropriate interpolator based on that determination. This can be changed to require a choice when the data that is to be interpolated is added.
The pointing interpolator has a location for the pointing data hardcoded into it that is used by default for now to avoid having to change code elsewhere. This scheme can be extended to have the place of pointing corrections, pedestals or gain at hand as well.
Tests have been included that check that the step interpolation works as expected.