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

Add other cameras to toy waveforms generator #1255

Closed
HealthyPear opened this issue Apr 6, 2020 · 7 comments
Closed

Add other cameras to toy waveforms generator #1255

HealthyPear opened this issue Apr 6, 2020 · 7 comments
Assignees

Comments

@HealthyPear
Copy link
Member

In testing new image extractors, it is possible that some of them at the beginning are optimized for a limited number of cameras (like the extractor in PR #1215, which is optimized for the moment for LSTCam and NectarCam).

The function camera_waveforms in ctapipe.image.tests.test_extractors.py seems to be fixed to work with SST-ASTRI/CHEC.

It would be useful to add also the other cameras for optional testing.

@kosack
Copy link
Contributor

kosack commented Apr 6, 2020

it would be good to have at least one hexagonal camera in there to make sure they work. Best would just be to use all cameras, or at least all CTA cameras (those that have a reference pulse at least).

@watsonjj
Copy link
Contributor

watsonjj commented Apr 7, 2020

At some point doesn't this become more of a benchmarking exercise than a unit test? The current tests ensures the methods run, and produce outputs you expect. The proposed changes are asking to make sure that camera-specific values (e.g. the tailcut thresholds used in #1215) are optimised.

I don't get the point about hexagonal cameras. Nothing about the ImageExtractor is specific to the pixel shape.

@kosack
Copy link
Contributor

kosack commented Apr 7, 2020

I was more thinking about less regular neighbor lists (for hex cameras) since the neighbors are used, but I guess it's not a problem

@kosack
Copy link
Contributor

kosack commented Apr 7, 2020

@HealthyPear if you want to test your new extractor using multiple cameras, you can of course do that. Otherwise I'll close this.

@kosack kosack closed this as completed Apr 7, 2020
@HealthyPear
Copy link
Member Author

I was asking because CHEC has only one channel, so I was thinking that testing with e.g LSTCam or NectarCam, which have 2 channels, would provide a more general case.

Nonethless, with SST-ASTRI/CHEC it seems that I managed to make the new ImageExtractor work within the precision set in the unit-test.

@watsonjj
Copy link
Contributor

watsonjj commented Apr 7, 2020

I was asking because CHEC has only one channel, so I was thinking that testing with e.g LSTCam or NectarCam, which have 2 channels, would provide a more general case.

It is now presumed that gain channel selection has occurred before image extraction. This was a change made in #1095

@maxnoe
Copy link
Member

maxnoe commented Apr 7, 2020

But you are producing waveforms, right? Not images.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants