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

Coordinates notebook corrected. #54

Merged
merged 7 commits into from Dec 18, 2018
Merged

Coordinates notebook corrected. #54

merged 7 commits into from Dec 18, 2018

Conversation

thomasgas
Copy link

Coordinate usage corrected after discussion with @maxnoe on the implementation of the coordinates with astropy. One should first create a frame and then create points in this frame with SkyCoord from astropy.
Related to cta-observatory/ctapipe#842
@maxnoe could you check this notebook? I'm doing the transformation from camera to AltAz explicitely for each frame.

@maxnoe
Copy link
Member

maxnoe commented Dec 4, 2018

I proposed some minor updates here:
https://github.com/thomasgas/cta-lstchain/pull/1

@thomasgas
Copy link
Author

thomasgas commented Dec 4, 2018

Thanks max for the review ;)
You can merge it now!

@maxnoe
Copy link
Member

maxnoe commented Dec 4, 2018

I think this belongs to ctapipe, you should also put it there

@thomasgas
Copy link
Author

I think this belongs to ctapipe, you should also put it there

Ok I'll do it.

@rlopezcoto
Copy link
Contributor

I would try to avoid duplicities, so if this is going to go to ctapipe maybe you can completely move it there?

@thomasgas
Copy link
Author

I would try to avoid duplicities, so if this is going to go to ctapipe maybe you can completely move it there?

I agree...I can remove it from here as long as the PR in ctapipe is merged.

@rlopezcoto
Copy link
Contributor

Sorry, I could not follow all the changes from last week... any progress on this point?

@thomasgas
Copy link
Author

I guess that i can remove the notebook since last week there has been a major coordinates refactoring. We can really remove it from here and there will just be the updated one in the ctapipe folder. let me do this and then you can merge the PR.

@thomasgas
Copy link
Author

ok you can merge the PR @rlopezcoto.

@rlopezcoto rlopezcoto merged commit ecd2b08 into cta-observatory:master Dec 18, 2018
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.

None yet

3 participants