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

CameraGeometry should store pixel coordinates as SkyCoord #416

Closed
kosack opened this issue May 23, 2017 · 16 comments
Closed

CameraGeometry should store pixel coordinates as SkyCoord #416

kosack opened this issue May 23, 2017 · 16 comments

Comments

@kosack
Copy link
Contributor

kosack commented May 23, 2017

I would like to store the camera coordinates inside CameraGeometry as a SkyCoord (not really sky, but that's the base class for astropy coordinates, even if confusingly named), but as of right now, this is difficult because when talking about a CameraGeometry, we do not necessarily have the focal length of the telescope in which the camera is installed.

Therefore, I would like to have CameraFrame be a base frame that does not need any parameters (it's just pixel pos in meters, relative to the center of the camera). Then to transform to an angular system where the pixels are positions on the sky (e.g. the TelescopeSystem), I would then add information about the focal length only. This system should be the same as CameraSystem, but angular coordinates, still centered on the center of the telescope's FOV.

Finally, I would later go to a AltAz or nominal by adding information on the pointing position of the telescope, etc.

@ParsonsRD is there a reason why it is not done this way? Right now I need to have the focal length to specify a CameraSystem coordinate, but I don't have that info, and it shouldn't be needed.

@kosack
Copy link
Contributor Author

kosack commented May 23, 2017

I guess, to me it makes sense like this:

 CameraFrame ---[telescope optics]---> TelescopeFrame ---[pointing]---> NominalFrame

@kosack
Copy link
Contributor Author

kosack commented May 23, 2017

This would then allow us to store the frame type in the CameraGeometry class (without loading a telescope optics description), and support the various frames described in #255

@ParsonsRD
Copy link
Member

Hi, sorry was on vacation for the last week. I think thats fine and shouldn't cause any serious problems.

@maxnoe
Copy link
Member

maxnoe commented Nov 21, 2017

I recently implented a working set of Coordinate Transformation for FACT here:
https://github.com/fact-project/pyfact/blob/master/fact/coordinates/camera_frame.py

I learned quite a lot about how astropy frames are supposed to work and what members they need to have to make transformation work painlessly.

I started working on implementing these in ctapipe, but right now I cannot give a timeframe when I will open a PR for this.

In the mean time, i'd like to know if there are definitions of the camera coordinate system and telescope coordinate system somewhere.
This would be very helpful for writing tests and the actual implementation.

@kosack
Copy link
Contributor Author

kosack commented Nov 21, 2017

I think @tino-michael is debugging that right now for his reconstruction, and found for example the extra 90 deg rotation that we are missing to go from the sim_telarray coord sys to the sky, etc. He's not using astropy coordinates yet, but once he has them finalized, they should be translated into them.

@kosack
Copy link
Contributor Author

kosack commented Nov 21, 2017

see #580

@labsaha
Copy link
Contributor

labsaha commented Nov 21, 2017

@maxnoe By definitions of the camera coordinate system and telescope coordinate system, you mean the frame of references for camera and telescope in which you have to provide FACT camera and telescope coordinates information to do the analysis using ctapipe, right?

@kosack is that the final ctapipe coordinate will be sim_telarray coordinate? or the sky one?

@maxnoe
Copy link
Member

maxnoe commented Nov 21, 2017

@maxnoe By definitions of the camera coordinate system and telescope coordinate system, you mean the frame of references for camera and telescope in which you have to provide FACT camera and telescope coordinates information to do the analysis using ctapipe, right?

No I mean stuff like: CameraCoordinates are defined as x pointing right and y pointing up, when one looks onto the camera plane from the mirror.

Or anything similiar. The definitions of what x and y should mean.

Is there somthing lik a CTA Document describing these coordinate frames?

@labsaha
Copy link
Contributor

labsaha commented Nov 21, 2017

I see. I don't know if any such CTA document exits. Others might have some more information on that. I see quite some discussion on coordinate systems in #255.

@kosack
Copy link
Contributor Author

kosack commented Nov 21, 2017

The standards for coordinate systems don't exist (and indeed each camera is doing things differently), so right now we only support the sim_telarray coordinate system, which is admittedly strange, and I couldn't even really find a reference for it (coordinate definitions are not mentioned in the sim_telarray manual - https://www.mpi-hd.mpg.de/hfm/~bernlohr/sim_telarray/Documentation/sim_hessarray.pdf). Eventually, we can standardize this better, and in fact the project office has been informed that this is a problem.

@kosack
Copy link
Contributor Author

kosack commented Nov 21, 2017

see also #580

@ParsonsRD
Copy link
Member

Hi, agreed @kosack no standards or requirements exist for this. The systems as they are, are a mish -mash between the HESS system, plus some other stuff I thought we might need. I agree this code definitely needs seriously refactoring but should we not try to fix the requirements (or at least get started on them) before we launch into a lot of work about this?

@AMWMitchell, from the PO side of thinks what do you think we need to do about fixing the requirements? Do you know if there is any existing group working on the coordinates standards?

Also, tagging @GernotMaier, what do you think?

@AMWMitchell
Copy link
Contributor

Yes, the PO is aware of the need for common pointing / co-ordinate definitions asap, and Gino Tosti agreed to start a document on this, but as yet I haven't seen any drafts and was considering starting one myself (with the FACT doc started by @maxnoe as input/reference as well as sorting out the mix of HESS / sim_telarray). Gino has ALMA/ASTRI experience, so between us (incl. those with VERITAS experience) we should have most variants covered.
Not sure which requirements you're thinking about - the only one relevant here would be to follow the applicable standards for co-ordinate definitions (TBD).

@ParsonsRD
Copy link
Member

Hi @AMWMitchell yeah requirements was a nomenclature fail on my part, agreed the only requirement should be follow the standards :-).

I just think it's probably worthwhile discussing the use cases for the coordinate frames to define what we actually need before launching into anything.

@tino-michael
Copy link
Contributor

I just think it's probably worthwhile discussing the use cases for the coordinate frames to define what we actually need before launching into anything.

Do you want to start a google doc or something to "brain-storm"?

dneise pushed a commit that referenced this issue Jan 23, 2019
* Start refactoring coordinates

* Update frame attributes for NominalFrame

* Remove weird module check

* Fix coordinate usage in muon code

* Set argv for tool tests, fixes error when using pytest options

* Add roudtrip test

* Names to pointings and docstrings.

* Renaming in tests and coordinates. no abbreviations.

* Fix kwargs

* More renames

* Use QuantityAttributes were appropriate

* Use default=0 for focal_length

* More coordinate fixing

* More coordinate fixes

* Fix coordinate trafo example

* Make HorizonFrame an alias to altaz

* Add test showing it is possible to go from ICRS to camera frame

* Add test that shows separation of planar frame is broken

* Add coordinate attribute back

* Implement nominal and telescopeframe like skyoffsetframe

* Fix imports

* Add missing rotation attribute, call it x and y again

* Allow transforming of AltAz into AltAz without obstime or location

* Use UnitSphericalRepresentation, add tests

* Use approx

* Use correct representation in camera to telescope

* Remove outdated examples

* Fix __all__

* Update docstrings

* Add example to plot stereo showers

* Ugly fix for doc warnings

* Fix invalid syntax

* Fix style issues

* Add test that shows transformation into camera is broken

* Wrap TelescopeFrame and NominalFrame angles at 180°

* Add comment about mapping function

* Rename Telescope and Nominal frame attributes to delta_az, delta_alt, fix camera coordinate trafo

* Do not set astropy config at import

* Remove comment about iers from docs

* Update nominal_frame.py

* Update telescope_frame.py

* Update docstring of camera coordinate

* Start reworking coord docs
watsonjj added a commit to watsonjj/ctapipe that referenced this issue Jan 23, 2019
* master:
  Major coordinate refactoring, fixes cta-observatory#899, cta-observatory#900, cta-observatory#505, cta-observatory#416 (cta-observatory#896)
  speed up is_compatible; makes event_source factory faster (cta-observatory#927)
  Prefix containers (cta-observatory#833)
  remove optional deps in order to fix tests (cta-observatory#925)
  Charge Resolution Update (cta-observatory#827)
  Remove serializer, fixes cta-observatory#887 (cta-observatory#913)
  deleted summary.html - accidentally committed file (cta-observatory#919)

# Conflicts:
#	ctapipe/tools/extract_charge_resolution.py
#	examples/simple_event_writer.py
@maxnoe maxnoe changed the title coordinates may need restructuring CameraGeoemtry should store pixel coordinates as SkyCoord Mar 20, 2020
@maxnoe maxnoe changed the title CameraGeoemtry should store pixel coordinates as SkyCoord CameraGeometry should store pixel coordinates as SkyCoord Mar 20, 2020
@maxnoe
Copy link
Member

maxnoe commented Oct 26, 2020

This is basically implemented with attaching frame to CameraGeometry and adding transform_to

@maxnoe maxnoe closed this as completed Oct 26, 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

No branches or pull requests

6 participants