Component class for adding pointing position using Drive Reports. #159

labsaha · 2019-08-03T22:40:47Z

This is the new PR following our previous discussion at #153. @FrancaCassol @vuillaut Please have a look. This is following the Component class as designed in ctapipe.

vuillaut

Hi
I don't really get the approach now...
If there is already a LSTDriveContainer that you are using, what is the need for another one here to contain (apparently?) the same info?

I thought the approach was globally:

read raw data with ctapipe_io_lst --> fill LSTDriveContainer
compute pointing position from the raw data --> update event.pointing

Let me know what I got wrong here.

lstchain/pointing/__init__.py

lstchain/pointing/pointings.py

labsaha · 2019-08-05T15:19:34Z

Hi Thomas @vuillaut Thanks for your comments

If there is already a LSTDriveContainer that you are using, what is the need for another one here to contain (apparently?) the same info?

I thought the approach was globally:

read raw data with ctapipe_io_lst --> fill LSTDriveContainer
compute pointing position from the raw data --> update event.pointing
Let me know what I got wrong here.

Perhaps, I didn't get your point. We wanted to have

a container class which we will fill with the values from drive report and that will be part of LSTMonitoringContainer https://github.com/cta-observatory/ctapipe_io_lst/blob/master/ctapipe_io_lst/containers.py
A Component class which will read the drive report (similar the reading pedestal file) and calculates the pointing position event-wise. This again following this one https://github.com/cta-observatory/cta-lstchain/blob/d62d44c5506422ba636e1e7b1622cb39417ef48d/lstchain/calib/camera/r0.py

I thought this is the way we wanted finally to proceed following our previous discussion in #153

vuillaut · 2019-08-05T15:29:13Z

Hi Thomas @vuillaut Thanks for your comments

If there is already a LSTDriveContainer that you are using, what is the need for another one here to contain (apparently?) the same info?

I thought the approach was globally:

read raw data with ctapipe_io_lst --> fill LSTDriveContainer
compute pointing position from the raw data --> update event.pointing
Let me know what I got wrong here.

Perhaps, I didn't get your point. We wanted to have

a container class which we will fill with the values from drive report and that will be part of LSTMonitoringContainer https://github.com/cta-observatory/ctapipe_io_lst/blob/master/ctapipe_io_lst/containers.py

A Component class which will read the drive report (similar the reading pedestal file) and calculates the pointing position event-wise. This again following this one https://github.com/cta-observatory/cta-lstchain/blob/d62d44c5506422ba636e1e7b1622cb39417ef48d/lstchain/calib/camera/r0.py

I thought this is the way we wanted finally to proceed following our previous discussion in #153

Right, my bad, I read the code too quickly and though it was a Container class.
Forget my comment.

vuillaut · 2019-08-09T10:56:33Z

Looks good to me but if @FrancaCassol could have a quick look that'd be perfect.

labsaha · 2019-08-30T15:50:17Z

@FrancaCassol Can you have a look into this issue?

FrancaCassol · 2019-09-02T09:10:20Z

@FrancaCassol Can you have a look into this issue?

Yes, sorry I will do it soon

FrancaCassol · 2019-09-03T13:22:42Z

Hi @labsaha,
I have some questions, comments:

could you better explain what is the "ev_time" array? (and write it in the description of the function)
perhaps I missed it, but I don't see where this component is used. Where are you going to use it? Could you provide an example?
you propose to create a directory for this component, is it supposed to contain something else than a component that is just doing an interpolation? (I must admit I still miss the meaning of this interpolation but perhaps I will understand it better after your reply to my previous questions). I ask because is seems to me this is just useful for filling the low level event containers, so, as already said, I find it more pertinent to the ctapipe_io_lst package
Finally, any new directory should contains its test directory with test modules, could you eventually provide it?

labsaha · 2019-09-04T16:07:36Z

Hi @FrancaCassol,
Thanks for you comments.

could you better explain what is the "ev_time" array? (and write it in the description of the function)

ev_time is the arrival time for each of the event in the data file. I assume that the event container will have information filled with timing for each of the event. Once we have that information that will be passed to this class to do the interpolation

perhaps I missed it, but I don't see where this component is used. Where are you going to use it? Could you provide an example?

It will be simply like this

from lstchain.pointing import PointingPosition
pointings = PointingPosition()
time=[1562283578, 1562333664, 1562333666]
Az,El=pointings.cal_pointingposition(time)

I don't know how you are handling timing information. If you have some code which is dealing with timing information for each of the events, please let me know. Then I can make a proper example file following this.

you propose to create a directory for this component, is it supposed to contain something else than a component that is just doing an interpolation? (I must admit I still miss the meaning of this interpolation but perhaps I will understand it better after your reply to my previous questions). I ask because is seems to me this is just useful for filling the low level event containers, so, as already said, I find it more pertinent to the ctapipe_io_lst package

We may not need a separate directory. But to make it clear that this directory is for code dealing with pointing information, I created it. I think there is no harm in having a directory like this at this stage. Then it will be easy for developers to find out the scripts associated with different separate purposes. I think I have created this component class following your suggestions. I am not sure if I understood you correctly then.

Finally, any new directory should contains its test directory with test modules, could you eventually provide it?

Yes, I can do this once the previous issues you mentioned are solved.

FrancaCassol · 2019-09-05T15:08:54Z

Hi @labsaha,

I am afraid your approach is not really ctapipe compliant.

from lstchain.pointing import PointingPosition
pointings = PointingPosition()
time=[1562283578, 1562333664, 1562333666]
Az,El=pointings.cal_pointingposition(time)

In ctapipe data are treated event wise, so you will not handle the time of several events at the time. Also, luckily you don't need it. Given that the driver position rate (0.5 Hz) is much slower than the event rate, for each event we need simply to find the time interval where the event occurred between two driver positions. From these two points we can simply linearly calculate the driver position at the event time (assuming the driver velocity is constant, but I think this is a reasonable assumption in a 2 second time interval during the data taking, what do you think @moralejo?). All this can be very easily done at the reader level as said several times.

I don't know how you are handling timing information. If you have some code which is dealing with timing information for each of the events, please let me know. Then I can make a proper example file following this.

ctapipe works event-wise, you find the timing information in the event containers (R0, R1, DL0). I strongly suggest you to have a deeper look at the structure of ctapipe and try the examples so to get accustomed to the capipe approach

We may not need a separate directory. But to make it clear that this directory is for code dealing with pointing information, I created it. I think there is no harm in having a directory like this at this stage. Then it will be easy for developers to find out the scripts associated with different separate purposes. I think I have created this component class following your suggestions. I am not sure if I understood you correctly then.

Actually, I suggested to create a Component only if it does something more than the linear interpolation between two points, otherwise it seems to me a bit overdone. Could you please verify with the driver people if there are corrections that must be applied on the values that they give to us or some post treatments to be eventually included in lstchain?

labsaha · 2019-09-05T16:55:52Z

@FrancaCassol

In ctapipe data are treated event wise, so you will not handle the time of several events at the time. Also, luckily you don't need it. Given that the driver position rate (0.5 Hz) is much slower than the event rate, for each event we need simply to find the time interval where the event occurred between two driver positions. From these two points we can simply linearly calculate the driver position at the event time (assuming the driver velocity is constant, but I think this is a reasonable assumption in a 2 second time interval during the data taking, what do you think @moralejo?). All this can be very easily done at the reader level as said several times.

Perhaps the example I have given considering a time array is not suitable one. But one can just use event-wise like event.time

Az,El=pointings.cal_pointingposition(event.time)
Then you fill the PontingPosition container of ctapipe with these two values.

ctapipe works event-wise, you find the timing information in the event containers (R0, R1, DL0). I strongly suggest you to have a deeper look at the structure of ctapipe and try the examples so to get accustomed to the capipe approach

Could you please specifically tell me which variable contains event-time in MC DL0 which is similar to what we will have as event time in the data file?

Actually, I suggested to create a Component only if it does something more than the linear interpolation between two points, otherwise it seems to me a bit overdone. Could you please verify with the driver people if there are corrections that must be applied on the values that they give to us or some post treatments to be eventually included in lstchain?

This is not just doing interpolation which is quick and easy but finding the time interval which is a bit expensive computationally and required to be done several times.

labsaha · 2019-10-18T13:21:27Z

After modifying the code on this PR a bit Daniel @morcuended has included the pointing positions on DL1 file of real data. He also included GPS time converted from the NTP time of the data file.

morcuended · 2019-10-23T15:43:19Z

Here is an example of an altitude-vs-gps_time plot for the run 1491 (32 subruns) that lasts around 100 minutes:

The altitude was calculated event-wise taking into account the gps_time values based on the NTP time stamps:
event.lst.tel[tel_id].svc.date + event.r0.tel[telescope_id].trigger_time,
and reading the corresponding drive report from /fefs/home/lapp/DrivePositioning/

morcuended

Hi @labsaha I think it is easier if you make the changes in this PR

morcuended · 2019-10-23T16:38:15Z

lstchain/pointing/pointings.py

+
+        """
+        timeL = ev_time[0]
+        timeH = ev_time[-1]


Since ev_time will be just a single value, I suggest deleting these two lines:

timeL = ev_time[0] timeH = ev_time[-1]

Sure. We don't need it anymore

morcuended · 2019-10-23T16:42:39Z

lstchain/pointing/pointings.py

+        drive_container.El_avg = data['El_avg'].data
+        xp = drive_container.time
+
+        ind = np.where((xp >= timeL) & (xp <= timeH))


In addition, the way to calculate the lower and upper elements of the drive time and their indices should be done like by adding these two lines:

lower_element = xp[xp < ev_time].max() upper_element = xp[xp > ev_time].min()

and replacing ind = np.where((xp >= timeL) & (xp <= timeH))
by:

ind = np.hstack( np.argwhere((xp >= lower_element) & (xp <= upper_element)) )

The rest keeps the same

Do you really need the indexes?
A mask will be faster

time_in_window = (xp >= timeL) & (xp <= timeH) run_times = xp[time_in_window]

Ok. We can modify accordingly.

vuillaut · 2019-10-25T14:42:42Z

lstchain/pointing/pointings.py

+        drive_container.El_avg = data['El_avg'].data
+        xp = drive_container.time
+
+        ind = np.where((xp >= timeL) & (xp <= timeH))


Do you really need the indexes?
A mask will be faster

time_in_window = (xp >= timeL) & (xp <= timeH) run_times = xp[time_in_window]

vuillaut · 2019-10-25T14:45:15Z

lstchain/pointing/pointings.py

+             A table of drive reports
+
+        """
+        from astropy.io import ascii


pep8 recommends making all imports at the beginning of the file

Ok. We'll put it at the beginning

lstchain/pointing/pointings.py

vuillaut · 2019-10-25T14:56:45Z

lstchain/pointing/pointings.py

+            run_Az = drive_container.Az_avg[ind]
+            run_El = drive_container.El_avg[ind]


pep8: no capital letter for variables

vuillaut · 2019-10-25T16:09:42Z

Do you want to update event.pointing with interpolated values in the same PR or a separated one?

labsaha · 2019-10-25T17:10:17Z

Do you want to update event.pointing with interpolated values in the same PR or a separated one?

I think we will do it in this PR

rlopezcoto · 2019-10-29T11:12:14Z

We will be needing this PR to be merged soon, @labsaha could you make the last changes to add the interpolated values so we can go ahead and merge? Thanks

labsaha · 2019-10-29T11:20:41Z

@rlopezcoto I got busy with some other things. I'll add the last changes today.

morcuended · 2019-10-29T16:19:02Z

lstchain/pointing/pointings.py

+    ).tag(config=True)
+
+    tel_id = Int(
+        0,


Shouldn't tel_id be 1 instead of 0 here?

I did it following the same piece of code defined in the component class of ctapipe

labsaha · 2019-10-29T17:29:43Z

@morcuended will make a new PR to show how this can be used to get the event.pointing

labsaha · 2019-10-29T17:43:17Z

Since some changes are suggested by @vuillaut following the PR at ctapipe_io_lst https://github.com/cta-observatory/ctapipe_io_lst/pull/11#issue-332586275, this script will work only if the PR by @vuillaut is merged.

Related to failed checks: We don't understand why it failed. It doesn't seem that the script is related to the failed checks.

rlopezcoto · 2019-10-30T09:08:40Z

I restarted the build and it worked. Since the ctapipe_io_lst PR was merged, is everybody is happy with the changes?
@morcuended, @labsaha are you then going to add the interpolated values for the pointing in a different PR?

labsaha · 2019-10-30T09:12:06Z

@rlopezcoto Yes, @morcuended will make a separate PR for the interpolated values.

vuillaut · 2019-10-30T09:16:07Z

I restarted the build and it worked. Since the ctapipe_io_lst PR was merged, is everybody is happy with the changes?

was it though? 😅
I am waiting for a review to merge it.

rlopezcoto · 2019-10-30T09:18:17Z

Grrrrr
Sorry, got a message from the repo of a PR merged and just assumed it was that one...

morcuended · 2019-11-06T09:15:47Z

PR we were waiting for was already merge. So this PR should be ready to merge as well.

vuillaut requested changes Aug 5, 2019

View reviewed changes

lstchain/pointing/__init__.py Outdated Show resolved Hide resolved

lstchain/pointing/pointings.py Outdated Show resolved Hide resolved

Updating Component class for adding pointing position

420d58f

labsaha force-pushed the pointing_position branch from c17ed19 to 420d58f Compare August 5, 2019 21:05

vuillaut approved these changes Aug 9, 2019

View reviewed changes

morcuended requested changes Oct 23, 2019

View reviewed changes

vuillaut requested changes Oct 25, 2019

View reviewed changes

vuillaut mentioned this pull request Oct 25, 2019

Pointing #195

Closed

morcuended reviewed Oct 29, 2019

View reviewed changes

updating script for pointing

ba16645

vuillaut mentioned this pull request Nov 4, 2019

Real data analysis #201

Merged

morcuended approved these changes Nov 6, 2019

View reviewed changes

rlopezcoto merged commit d360590 into cta-observatory:master Nov 6, 2019

		run_Az = drive_container.Az_avg[ind]
		run_El = drive_container.El_avg[ind]

Component class for adding pointing position using Drive Reports. #159

Component class for adding pointing position using Drive Reports. #159

Conversation

labsaha commented Aug 3, 2019

vuillaut left a comment

Choose a reason for hiding this comment

labsaha commented Aug 5, 2019 • edited Loading

vuillaut commented Aug 5, 2019

vuillaut commented Aug 9, 2019

labsaha commented Aug 30, 2019

FrancaCassol commented Sep 2, 2019

FrancaCassol commented Sep 3, 2019

labsaha commented Sep 4, 2019

FrancaCassol commented Sep 5, 2019

labsaha commented Sep 5, 2019

labsaha commented Oct 18, 2019

morcuended commented Oct 23, 2019

morcuended left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vuillaut commented Oct 25, 2019

labsaha commented Oct 25, 2019

rlopezcoto commented Oct 29, 2019

labsaha commented Oct 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

labsaha commented Oct 29, 2019

labsaha commented Oct 29, 2019

rlopezcoto commented Oct 30, 2019

labsaha commented Oct 30, 2019

vuillaut commented Oct 30, 2019

rlopezcoto commented Oct 30, 2019 • edited Loading

morcuended commented Nov 6, 2019

labsaha commented Aug 5, 2019 •

edited

Loading

rlopezcoto commented Oct 30, 2019 •

edited

Loading