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

Array plotting #784

Merged
merged 10 commits into from
Oct 10, 2018
Merged

Array plotting #784

merged 10 commits into from
Oct 10, 2018

Conversation

thomasgas
Copy link

@thomasgas thomasgas commented Sep 12, 2018

This PR is a connected to some other PRs (#723, #737 and #771).
I recently merged #771 where i corrected the angle (phi to psi) used to plot the arrow on the ground, changing the arrow with a line.
Since the arrow was not being plotted correctly, I worked a bit on a correct plotting for the arrow on the ground and I fixed some bugs in a already existing function used to calculate the time gradient on some selected pixels (usually those selected after the cleaning).
I also added an example notebook (thanks to @vuillaut for providing an example to work on) which displays both the cameras (image + hillas, peakpos and selected region which is being considered for the computation of the gradient) and the arrows on the ground (only if the gradient is not zero, otherwise only the line from the hillas parametrization can be plotted).
Here's an image of how it looks like now (from the example file in ctapipe):
arrow_test
I also added an option to the labels in order to plot only the labels inside the desired region (and not all of them as it was before even only a smaller region was selected).
Since the arrows had a strange length, i added some options and changed a bit the code to make it similar to the line plotting: now the length of the arrow can be selected manually.
Open to suggestions and improvements.

Thomas Gasparetto added 6 commits September 11, 2018 16:10
- labels are not plotted is they are outside the selected region
- plotting arrows considering the timing parameters
- functions are now working for diffuse, point, N & S pointing simtel files
- arrows had a not very clear behaviour. Changed to a simpler one (fixed length).
@codecov
Copy link

codecov bot commented Sep 12, 2018

Codecov Report

Merging #784 into master will increase coverage by <.01%.
The diff coverage is 83.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
+ Coverage   71.54%   71.54%   +<.01%     
==========================================
  Files         199      199              
  Lines       10824    10843      +19     
==========================================
+ Hits         7744     7758      +14     
- Misses       3080     3085       +5
Impacted Files Coverage Δ
ctapipe/image/tests/test_timing_parameters.py 100% <ø> (ø) ⬆️
ctapipe/visualization/tests/test_mpl.py 100% <100%> (ø) ⬆️
ctapipe/visualization/mpl_array.py 96.07% <66.66%> (-2.91%) ⬇️
ctapipe/image/timing_parameters.py 93.93% <85.71%> (-6.07%) ⬇️

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 72d9b90...69252c5. Read the comment docs.

Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

Does this plotting still work if you can't yet calculated the time gradient dictionary? It would be nice if it can still be used if not (e.g. have it still do something sensible if None is passed for time_gradient.

@thomasgas
Copy link
Author

I think that we should plot the arrow only if we can be confident about which direction we are watching at: if we are not confident about this, then the line plotting function should be used.
If you think that the arrow plotting still make sense without the time gradient, I can add the None option to time_gradient.

@kosack
Copy link
Contributor

kosack commented Sep 21, 2018

You can get the direction from the geometric asymmetry as well as the time gradient, so that could be used as well - it would be nice to show either one. The main reason is that we are not guaranteed to have time information, e.g. when we look at data from HESS or early Astri camera simulations

@kosack
Copy link
Contributor

kosack commented Sep 21, 2018

You could also have it just draw lines, not vectors, in the case that the time_gradient is not given

@thomasgas
Copy link
Author

I tried to use the skewness to have the information regarding the time gradient but I don't get really nice results. I think that for the moment I can make the function draw an arrow if the gradient is given, and draw lines if the gradient is either not given (the time_gradient = None option) or if it's zero for some telescopes.

@maxnoe
Copy link
Member

maxnoe commented Oct 9, 2018

I tried to use the skewness to have the information regarding the time gradient but I don't get really nice results. I think that for the moment I can make the function draw an arrow if the gradient is given, and draw lines if the gradient is either not given (the time_gradient = None option) or if it's zero for some telescopes.

You could just give it a direction keyword argument containing 1, 0, -1 for the three possibilities for each telescope, and the user can decide what to use.

@thomasgas
Copy link
Author

You could just give it a direction keyword argument containing 1, 0, -1 for the three possibilities for each telescope, and the user can decide what to use.

So @maxnoe: +1 is the arrow, -1 is the flipped arrow and 0 is the line? and it's up to the user decide which is the discriminator (time, skewness, or whatever)?

@maxnoe
Copy link
Member

maxnoe commented Oct 9, 2018

Yes, that was my idea. What do you think?

@thomasgas
Copy link
Author

Yes, that was my idea. What do you think?

I think that having this as more general as possible is better: I can add a use case with the sign of the time gradient in the notebook that I created.

@kosack kosack merged commit 4422cb7 into cta-observatory:master Oct 10, 2018
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request Nov 9, 2018
* master: (60 commits)
  Add test that shows slicing breaks cam geom and fix it (cta-observatory#782)
  fix ctapipe build failure (cta-observatory#811)
  fix package name for yaml (should be pyyaml) (cta-observatory#810)
  Implement number of islands (cta-observatory#801)
  fixed ranges of cam-display so they correspond to fixed toymodel sims (cta-observatory#808)
  Fix unknown section example warning (cta-observatory#800)
  Fix timing parameters for case when there are negative values in image (cta-observatory#804)
  Update Timing Parameters (cta-observatory#799)
  speed up unit tests that use test_event fixture (cta-observatory#798)
  Add unit to h_max in HillasReconstructor (cta-observatory#797)
  Codacy code style improvements (cta-observatory#796)
  Minor changes: mostly deprecationwarning fixes (cta-observatory#787)
  Array plotting (cta-observatory#784)
  added a config file for github change-drafter plugin (cta-observatory#795)
  Simple HESS adaptations (cta-observatory#794)
  add test for sliced geometries for hillas calculation (cta-observatory#781)
  Impact intersection (cta-observatory#778)
  updated main documentation page (cta-observatory#792)
  Implement concentration image features (cta-observatory#791)
  Fix bad builds by changing channel name (missing pyqt package) (cta-observatory#793)
  ...

# Conflicts:
#	ctapipe/calib/camera/dl1.py
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