-
Notifications
You must be signed in to change notification settings - Fork 269
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
Drop support for CameraFrame in ImageProcessor #2141
Closed
Closed
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
d0b8083
remove CameraFrame from hillas
StFroese 8034cee
remove CameraHillasParametersContainer option
StFroese a004399
extraction in TelescopeFrame
StFroese 430fed2
remove CameraHillasParametersContainer option
StFroese 26068fd
toymodels in TelescopeFrame
StFroese ed0f266
tests in TelescopeFrame
StFroese a0a9ee3
image processing in TelescopeFrame only
StFroese 61005fd
remove CameraFrame option
StFroese 67a9143
test in TelescopeFrame
StFroese 765e50b
remove test for CameraFrame
StFroese a6eb055
create ToyEventSource in TelescopeFrame
StFroese 3440e85
remove comparison between CameraFrame and TelescopeFrame
StFroese 9b93b91
remove CameraFrame
StFroese 37fe0e7
remove CameraHillasParametersContainer
StFroese 2bc2b3e
speedup using frame instead of SkyCoord
StFroese 9af1cf7
Merge branch 'master' into remove_hillas_cameraframe
StFroese 2f3c6de
pixel_positions for TelescopeFrame
StFroese 712e436
use TelescopeFrame in doc notebooks
StFroese 84ceeb7
Merge branch 'master' into remove_hillas_cameraframe
StFroese de32018
fix CameraDisplay test
StFroese 72c33b2
move geometry transformation to setup
StFroese 3128997
subarray transform to TelescopeFrame
StFroese ce99544
Merge branch 'master' into remove_hillas_cameraframe
StFroese File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might need some thought - we shouldn't be transforming the geometry for each event inside a calibration/image-extraction algorithm like this as that adds quite a bit of overhead. This algorithm does not care about the frame of the camera geometry or pointing corrections, so it should not require a transform.
Since we do eventually need to transform for the ImageProcessor, it may be better to do the transform outside of these algorithms, perhaps in the CameraCalibrator (which eventually should also apply pointing correcions), so that a single TelescopeFrame is available to all algorithms that need it. That would avoid re-doing the transform many times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be done inside the
ProcessorTool
insetup
after writing out the subarray description but before setting up telescope components. The telescope components should just get a subarray with cameras inCameraFrame
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add
transform_camera_geometries
toSubarrayDescription
returning a new Subarray with transformed geometriesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is creating a whole new SubarrayDescription each time efficient? This was an issue I think we discussed a while back (need to find where): during analysis, the TelescopeFrame needs to be updated once per event for observations, and could be simplified to only once per run for simulations in fixed Alt/Az mode.
The TelescopeFrame requires:
Therefore where to store these frames is a question. Normally the SubarrayDescription is a static thing, but it could go there, but re-generating a whole SubarrayDescription might be wasteful if we could just update the info. Note that we don't want to re-generate neighbor pixel info in
CameraGeometry
for each event, as that does not change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not each time. Once in the tool in
setup
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, forgot about pointing corrections.
But it stands: we could also do the pointing corrections in the transformation from / to TelescopeFrame to higher frames. That would actually make the most sense to me, since pointing direction is not important for the
CameraFrame
<->TelescopeFrame
transformation.To me that means we should transform the whole subarray once to
TelescopeFrame
and apply corrections in the transformationAltAz
<->TelescopeFrame
, we don't need to change telescope frame for this, as the attributes needed for the transformation can be specified on theSkyCoord
as seen here:ctapipe/ctapipe/visualization/mpl_camera.py
Lines 434 to 439 in 8a7a367
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember there are in principle rotation corrections and maybe others, so you have to apply pointing corrections before Hillas parameterization (doing it while going to Alt/Az wouldn't work), as you need to have correctly shifted/rotated parameters before you do shower geometry reconstruction. But that can be solved when the ShowerProcessor is moved to TelescopeFrame later (right now e.g. HillasReconstructor supports both frames, but that's ugly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxnoe @kosack please have a look now if this solves the issue