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

Wrong angle in ArrayDisplay. changed phi to psi. #771

Merged
merged 7 commits into from
Aug 22, 2018
Merged

Wrong angle in ArrayDisplay. changed phi to psi. #771

merged 7 commits into from
Aug 22, 2018

Conversation

thomasgas
Copy link

@thomasgas thomasgas commented Jul 31, 2018

I started to look at the plotting code in ArrayDisplay, since it was not working that well for point gammas and really giving random results for diffuse gammas and protons (as @vuillaut pointed out recently in #721 ).
The results of my investigation is that the angle used for plotting was the wrong one, so i changed the phi angle to psi angle: these angles are very similar for point sources (that's why the plots for point sources don't look that bad) but very different for other sources.
I'm now working on a simple algorithm for plotting the arrow in the right direction since, as pointed out by @kosack in #723 and #737 sometimes we have a 180° flip.
I analyzed the same file as @vuillaut and this is what I get using his notebook:
protons

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2018

How about stop using greek letters and use meaningful names like shower_axis_angle and cog_polar_angle?

@thomasgas
Copy link
Author

I agree @maxnoe!
I will change some names to more useful ones. I'll change them names using the description in ctapipe.io.containers.HillasParametersContainer and your suggestions:

  • phi --> cog_polar_angle
  • psi --> rot_angle_ellipse

is that better?

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2018

Sounds good, but let's do another PR for this so we can discuss this separately

@thomasgas
Copy link
Author

so this PR can be closed and we leave the name changes to another PR.

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2018

I meant to disentangle the fix for the display from the change of variable names. So this PR fixes? the display and another renames variables.

@thomasgas
Copy link
Author

thomasgas commented Aug 1, 2018

This PR just fixes the display, replacing phi with psi. we can either close it ore I can also make some changes in the variables...what do you think?

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2018

I wouldn't close it but I also wouldn't add more changes here ;)

Let's keep it as it is and Karl can decide if he wants to merge it or if he has an idea how to fix this 180 deg offset first.

@thomasgas
Copy link
Author

i'm presently working on a simple algorithm to plot the arrows in the right direction, using the time gradient of the images. I'll add it to this PR asap :)

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2018

Ah, no I get what the problem is.

It's the classical problem of the disp approach, that you do not know, in which direction on the main shower axis the true source lies, right?

So we probably shouldn't plot arrows here but lines in both possible directions?

Also, is the direction in this view actually well-defined? The shower axis is something along the camera coordinates, how is this translated to the array top view?

@vuillaut
Copy link
Member

vuillaut commented Aug 1, 2018

Hi @maxnoe and @thomasgas.

So we probably shouldn't plot arrows here but lines in both possible directions?

I agree with you, for each telescope, we don't know the pointing direction.

However, after reconstruction, we do. This info could be used to draw arrows in the right direction but that would assume the stereoscopic reconstruction has been done and that it is relatively exact. so I am not sure this is something we want for a display tool - and even if we do that should go in another PR.

So I propose for this PR:

  • [done] correct the angle issue
  • remove arrows entirely (replace them with lines)

Then in other PRs:

  • change names in Hillas
  • [optional] add arrows in the case the stereo reconstruction has been done or using the timing for each telescope

Thank you @thomasgas for finding this bug.

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2018

add arrows in the case the stereo reconstruction has been done or using the timing for each telescope

The skewness could also be used.

In generall, there could be a Classifier choosing from multiple variables, which is the correct direction, this is how it is done in the current FACT analysis.

@vuillaut
Copy link
Member

vuillaut commented Aug 1, 2018

The skewness could also be used.
In generall, there could be a Classifier choosing from multiple variables, which is the correct direction, this is how it is done in the current FACT analysis.

Yes absolutely but this is still in the reconstruction part and not necesseraily related to the display.
Not that the display of the directions could be done without any reconstruction.

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2018

Yes absolutely but this is still in the reconstruction part and not necesseraily related to the display.
Not that the display of the directions could be done without any reconstruction.

How that? Maybe I don't know what this plot should show.

I was under the impression, that this should show the direction to the source position.

@vuillaut
Copy link
Member

vuillaut commented Aug 1, 2018

@maxnoe
Well, in my opinion, it shows the direction given by each telescope, for example, to visually check if the intersection of the lines corresponds with the true shower impact position.

Maybe I should have been clearer when I said no reconstruction, I meant not "DL2 reconstruction" (therefore no stereoscopy, no reconstruction of the source, etc...). The display just needs the direction given by Hillas, done at "DL1 level" and per telescopes.

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2018

Well, in my opinion, it shows the direction given by each telescope, for example, to visually check if the intersection of the lines corresponds with the shower impact position.

But is this really as simple as taking the hillas angle and plotting it on a differently oriented 2d plane with an offset? I don't think this is correct. I have to think about what the main axis in the camera plane implies for a top-down view of the array.

@thomasgas
Copy link
Author

As you can see i added a very simple function which just plots a segment for each telescope and a point at the center (better visualization in my opinion).
With this code:

array = ArrayDisplay(event.inst.subarray.select_subarray("Paranal", layout))
array.set_line_hillas(hillas_try, range = 300, linewidth=3)  # linewidth is optional

the same event now looks something like this:
protons_lines

I just changed the names of the variables only in the functions set_vector_hillas and in this new one.

@moralejo
Copy link
Contributor

moralejo commented Aug 2, 2018 via email

@maxnoe
Copy link
Member

maxnoe commented Aug 2, 2018

As you can see i added a very simple function which just plots a segment for each telescope and a point at the center (better visualization in my opinion).
With this code:

That looks much better! Congrats. And it shows that the Hillas reconstruction works, right? The cross is the true impact, correct?

@thomasgas
Copy link
Author

As @moralejo pointed out we need to check whether this plotting works also for large zenith angles (i think the projection needs be handled better in the plotting tool, but maybe someone else can comment on that).
Here you are a plot for a diffuse gamma at La Palma (this is a really bright event...not all of them looks this good!!!)
la_palma_diffuse_2_lsts
Thanks @maxnoe! yes that is the true impact point! The telescopes that are not pointing in "right" direction are at the edge of the camera...so it seems like the plotting is not that bad, but it needs to be checked with other simulations.

@kosack
Copy link
Contributor

kosack commented Aug 21, 2018

Thanks all for this - it looks like a major improvement.

For the question of arrows, we could (perhaps optionally) plot an arrow head in the direction of the spatial assymmetry. In the future we could also include the time parameters as Thomas suggested, so if you give it both Hillas and Timing parameters, we could plot both the time gradient direction and the spatial assymmetry.

@kosack
Copy link
Contributor

kosack commented Aug 21, 2018

The test_array_display unit test is still failing. Seems to be a unit problem in ad.set_vector_hillas(hillas_dict) when given the following as input:

   hillas_dict = {
            1: HillasParametersContainer(length=1.0 * u.m, phi=90 * u.deg),
            2: HillasParametersContainer(length=200 * u.cm, phi="95deg"),
        }

@kosack
Copy link
Contributor

kosack commented Aug 21, 2018

Also, I definitely agree on the variable name change as well. Clearly they were ambiguous, since I used the wrong one!

@kosack
Copy link
Contributor

kosack commented Aug 21, 2018

I'd suggest for the renaming PR, to be consistent in the naming like:

  • phi --> cog_polar_angle
  • psi --> ellipse_rotation_angle

At least to have "angle" be the last part of the name consistently

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.

please fix unit test, but otherwise read to merge

@thomasgas
Copy link
Author

I'll fix the PR in order to make it pass all the tests.

For the question of arrows, we could (perhaps optionally) plot an arrow head in the direction of the spatial assymmetry. In the future we could also include the time parameters as Thomas suggested, so if you give it both Hillas and Timing parameters, we could plot both the time gradient direction and the spatial assymmetry.

@kosack I'm now testing a script to find the time gradient and to plot it on the camera image. I'll make a new PR when it will be ready and give good results (almost ready).

@codecov
Copy link

codecov bot commented Aug 21, 2018

Codecov Report

Merging #771 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #771      +/-   ##
==========================================
+ Coverage   70.95%   70.99%   +0.04%     
==========================================
  Files         198      198              
  Lines       10691    10706      +15     
==========================================
+ Hits         7586     7601      +15     
  Misses       3105     3105
Impacted Files Coverage Δ
ctapipe/visualization/mpl_array.py 98.97% <100%> (+0.17%) ⬆️
ctapipe/visualization/tests/test_mpl.py 100% <100%> (ø) ⬆️

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 6a2a6a8...557c98e. Read the comment docs.

@kosack kosack merged commit 77aaf44 into cta-observatory:master Aug 22, 2018
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants