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

Improve ArrayDisplay and SubarrayDescription #723

Merged
merged 42 commits into from
May 3, 2018

Conversation

kosack
Copy link
Contributor

@kosack kosack commented Apr 24, 2018

The visualization.ArrayDisplay code was quite out of date, and mostly not useful the way it was implemented. However, having such a display is critical for debugging reconstruction code. This PR simplifies it, makes it use the SubarrayDescription correctly, and adds some useful functionality. It so far will break the plotting.ArrayPlotter, but that seems to be an unnecessary class and will be replaced with basic ArrayDisplay functionality.

ArrayDisplay Changes:

  • Three things can be controlled: the telescope border color, the telescope size, and the fill color:
    • telescope sizes are back to being set to the mirror radius by default (that functionality seems to have disappeared in a previous refactoring for some reason), but you can set them to all the same size if you like.
    • telescope borders are colored by telescope type by default
    • the internal fill color can be controlled by setting the values a-tribute, similar to CameraDisplay.image.
  • You now can overlay a vector field (e.g. for HillasParameters) in a way that updates very quickly (internally uses a matplotlib.quiver object). This is controlled by the set_vector_uv(), set_vector_rho_phi() or set_vector_hillas() functions, all of which provide different ways to set the vector angle and length
  • added examples/array_display.py that shows an animated display
  • added examples/plot_array_hillas.py that plots the array, the core position, and the hillas ellipses for each event
  • supports other coordinate frames (default is GroundFrame)

SubarrayDescription changes:

  • telescope coordinates now stored as an astropy SkyCoord in the GroundFrame
  • deprecated the .pos_x, .pos_y, .pos_z aattributes, and now recommend using .tel_coords
  • added some helper functions to map tel_ids to indices, etc.
  • added telescope_types, camera_types, and optics_types atributes, that give back lists of those quantities, helpful for selecting telescopes.
  • added get_tel_ids_for_type() helper, that returns a list of tel_ids for a given telescope type.

Some things that need improvement:

  • need support for different coordinate Frames. E.g. should be able to give ArrayDisplay a Frame, and have it convert the SubarrayDescription telescope positions automatically, so that the result is e.g. in the TiltedFrame
  • need to add some functionality from ArrayPlotter, and remove/deprecate ArrayPlotter

Some problems possible unrelated to this PR, but that should be looked into:

  • not sure right now why the Hillas vectors seem to intersect nicely at the core position in some MC files I tried, and not in others where the core position is far away form the intersection point (could be due to the coordinate frame, but it seems larger than the zenith angle offset). See e.g. the plot below:
  • related to previous question, it seems in some MC files, the vector phi angle is 180 degs off. No idea why that would happen - it should always correspond to the assymmetry direction, I guess. Could simply plot the vectors as lines (2 vectors in opposite directions). Could also be coordinates - seems to be different for North and South arrays.

A LaPalmaRefSim MC:
image

A standard Prod3b MC:
image

@kbruegge
Copy link
Member

not sure right now why the Hillas vectors seem to intersect nicely at the core position in some MC files I tried, and not in others where the core position is far away form the intersection point (could be due to the coordinate frame, but it seems larger than the zenith angle offset). See e.g. the plot below:
related to previous question, it seems in some MC files, the vector phi angle is 180 degs off. No idea why that would happen - it should always correspond to the assymmetry direction, I guess. Could simply plot the vectors as lines (2 vectors in opposite directions). Could also be coordinates - seems to be different for North and South arrays.

Could this be related to #721 ?

@maxnoe
Copy link
Member

maxnoe commented Apr 25, 2018

I had a nice laugh when I read:

SkyCoord in the GroundFrame

But to be fair, these are to completely different things. A telescope position is something absolute on Earth, not in the Sky. While Telescope Pointings and positions in the camera / fov are SkyCoords, telescope positions are just EarthLocations. But I think this is more a topic for the rework of the coordinates module.

@kosack
Copy link
Contributor Author

kosack commented Apr 27, 2018

But to be fair, these are to completely different things. A telescope position is something absolute on Earth, not in the Sky. While Telescope Pointings and positions in the camera / fov are SkyCoords, telescope positions are just EarthLocations. But I think this is more a topic for the rework of the coordinates module.

Yeah, I had the same thought, but it's just that AstroPy decided to call their coordinate class SkyCoord, and not just Coord, which may have made more sense... We don't have control over that. We could use EarthLocations for the telescope positions, but then it doesn't give you the nice ability to transform - right now, there is a single EarthLocation, which is the center of the array, and then the GroundFrame defines a coordinate system around that point where the telescopes are. Anyhow, the whole set of coordinate systems needs some redesign. The fact is however, that we really do want to consider the array ground positions as positions on the sky - that's how the reconstruction is done, and the ability to turn them into Alt/Az coordinates on a sphere is super useful for reco and calibration purposes.

@codecov
Copy link

codecov bot commented Apr 27, 2018

Codecov Report

Merging #723 into master will increase coverage by 0.81%.
The diff coverage is 90.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #723      +/-   ##
==========================================
+ Coverage   68.15%   68.96%   +0.81%     
==========================================
  Files         196      196              
  Lines       10548    10522      -26     
==========================================
+ Hits         7189     7257      +68     
+ Misses       3359     3265      -94
Impacted Files Coverage Δ
ctapipe/plotting/event_viewer.py 0% <0%> (ø) ⬆️
ctapipe/image/muon/muon_reco_functions.py 0% <0%> (ø) ⬆️
ctapipe/plotting/array.py 0% <0%> (-19.52%) ⬇️
ctapipe/visualization/tests/test_mpl.py 100% <100%> (ø) ⬆️
ctapipe/visualization/__init__.py 71.42% <100%> (+11.42%) ⬆️
ctapipe/instrument/tests/test_subarray.py 100% <100%> (ø) ⬆️
ctapipe/visualization/mpl_camera.py 79.88% <100%> (ø)
ctapipe/io/hessioeventsource.py 95.12% <100%> (ø) ⬆️
ctapipe/instrument/subarray.py 78.89% <78.12%> (+0.58%) ⬆️
ctapipe/visualization/mpl_array.py 98.8% <98.8%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 275e5a5...50f5561. Read the comment docs.

@kosack kosack merged commit cc8795d into cta-observatory:master May 3, 2018
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request May 4, 2018
* master:
  Correct dl1.py for using inst.num_channels for the integration correction (cta-observatory#730)
  Muon reco changes (cta-observatory#736)
  Changes to TargetIOEventSource (cta-observatory#732)
  Improve ArrayDisplay and SubarrayDescription (cta-observatory#723)

# Conflicts:
#	ctapipe/io/containers.py
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request May 4, 2018
* master:
  install protozfits v0.44.5 and require it for nectarcam (cta-observatory#734)
  Correct dl1.py for using inst.num_channels for the integration correction (cta-observatory#730)
  Muon reco changes (cta-observatory#736)
  Changes to TargetIOEventSource (cta-observatory#732)
  Improve ArrayDisplay and SubarrayDescription (cta-observatory#723)

# Conflicts:
#	ctapipe/io/containers.py
#	ctapipe/io/tests/test_nectarcameventsource.py
@thomasgas thomasgas mentioned this pull request Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants