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

Fixing the toy image generator #790

Merged
merged 10 commits into from
Oct 8, 2018

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Sep 27, 2018

When implementing the concentrations features, I found that the toy image generator was severely broken.

  • The given intensity is not reproduced, the showers are much, much too bright.

  • Width and Length are taken as variance, not standard deviation

  • The cleaning for the test event is far from optimal.

Here is the image that is used for the hillas tests:

example_after

The maximum pixel value is over 9000!
Just kidding, it's over 2000.

* The evaluation of the 2d gaussian was missing the normalisation
for the pixel area
* Length and width were not squared
* Adapted test image to show a more realistic shower image
@maxnoe
Copy link
Member Author

maxnoe commented Sep 27, 2018

This is the test image after the fix:
example_after

@maxnoe
Copy link
Member Author

maxnoe commented Sep 27, 2018

The hillas tests are now failing because some methods report a psi rotated by 180 degrees.

@@ -12,19 +12,33 @@
cam_ids = CameraGeometry.get_known_camera_names()


def create_mock_image(geom):
Copy link
Member

@dneise dneise Sep 27, 2018

Choose a reason for hiding this comment

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

maybe one could expose the "magic numbers" as parameters of this function, like?

def create_mock_image(
    geom,
    centroid=0.3,
    width=0.03,
    length=0.10,
    psi="25d",
    intensity=0.5,
    nsb_level_pe=3,
):

Copy link
Member

Choose a reason for hiding this comment

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

or maybe one could somehow comment on this particular choice for these 6 parameters?

Copy link
Member Author

Choose a reason for hiding this comment

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

The function is defined in the test, so is not a user facing function. The whole does also not work with astropy units. So if we would like to generalize this function, more work is needed anyway.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@maxnoe
Copy link
Member Author

maxnoe commented Sep 27, 2018

The tests are passing again, because I coerced the psi angles in the test to the [-90, 90] interval.

Maybe we should think about adding this in the hillas_functions themselves but we will also not keep 5 hillas_functions, right?

@codecov
Copy link

codecov bot commented Sep 27, 2018

Codecov Report

Merging #790 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #790      +/-   ##
==========================================
+ Coverage    71.7%   71.76%   +0.05%     
==========================================
  Files         201      202       +1     
  Lines       10939    10968      +29     
==========================================
+ Hits         7844     7871      +27     
- Misses       3095     3097       +2
Impacted Files Coverage Δ
ctapipe/image/tests/test_geometry_converter.py 100% <100%> (ø) ⬆️
ctapipe/image/toymodel.py 100% <100%> (ø) ⬆️
ctapipe/image/tests/test_hillas.py 100% <100%> (ø) ⬆️
ctapipe/image/tests/test_toy.py 100% <100%> (ø)
ctapipe/io/nectarcameventsource.py 95.23% <0%> (-4.77%) ⬇️

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 603d1ab...f7335c5. Read the comment docs.

@maxnoe maxnoe force-pushed the fix_toy_event_generator branch from 1764333 to f7335c5 Compare September 27, 2018 18:42
@maxnoe
Copy link
Member Author

maxnoe commented Sep 28, 2018

Somehow the installation of pyqt fails on travis for python 3.6

@kosack
Copy link
Contributor

kosack commented Oct 8, 2018

Thanks - making the toy model be actually useful and in correct units has been on the todo list for a long time... it got kind of forgotten once we had simulation data.

Somehow the installation of pyqt fails on travis for python 3.6

It's annoying that we even need to install it since it's not used, but I think it's a matplotlib dep in Anaconda's build of matplotlib? In any case, that can be fixed in another PR.

@kosack kosack merged commit 4f79323 into cta-observatory:master Oct 8, 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.

3 participants