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

Updating files for the correction to pointing positions #272

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

labsaha
Copy link
Collaborator

@labsaha labsaha commented Jan 23, 2020

This is for adapting the changes required for pointing information.

@@ -67,7 +67,7 @@ def cal_pointingposition(self, ev_time, drive_data):
drive_container = LSTDriveContainer()
drive_container.time = drive_data['time'].data
drive_container.azimuth_avg = drive_data['azimuth_avg'].data
drive_container.zenith_avg = drive_data['zenith_avg'].data
drive_container.altitude_avg = 90.0 - drive_data['zenith_avg'].data
Copy link
Member

Choose a reason for hiding this comment

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

what object is drive_data?
docstring only states "container".

Following questions:

  • does it contain zenith_avg?
  • are we sure it is in degrees?

Copy link
Collaborator Author

@labsaha labsaha Jan 23, 2020

Choose a reason for hiding this comment

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

drive_data is the data table

  • yes, drive_data contains zenith_avg
  • Yes we are sure that they are in degress

Copy link
Member

Choose a reason for hiding this comment

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

I am sorry, it does not actually answer my question, what type of object is it? ctapipe.io.container?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I am not getting you properly. I am sorry for that. I think you can easily figure this out.

Copy link
Collaborator Author

@labsaha labsaha Jan 23, 2020

Choose a reason for hiding this comment

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

Drivereport: Container perhaps I need to change this in the docstring

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you talking about drive_container or drive_data?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, since this is needed for the reprocessing, I'll merge it and if there are some clarifications to be included in the docstring, they can be included in a subsequent PR.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am not getting you properly. I am sorry for that. I think you can easily figure this out.

Well if I could I would not ask.
This function receives an object called drive_data of unknown type (Drivereport: Container is pointing to what ? where is it defined?) this kind of info is essential to make a review or simply later fix bugs, use the function...

@rlopezcoto
Copy link
Contributor

Following the discussion of #26 in ctapipe_io_lst, to follow ctapipe convention, instead of zenith we will use altitude

@rlopezcoto rlopezcoto merged commit 30daca6 into cta-observatory:master Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants