changes for parallel and non parallel pointings #38
Conversation
protopipe/pipeline/event_preparer.py
Outdated
| # pointing to predict | ||
| for tel_id in tels_pointing: | ||
| if tels_pointing[tel_id].alt != event.mcheader.run_array_direction[1]: | ||
| tels_pointing = None |
There was a problem hiding this comment.
should you really set your loop variable to None inside the loop? That seems to be bad form. This is just checking that the telescopes are parallel right? I would suggest either using something like
all(x.alt==dir[1] and x.az==dir[0] for x in tels_pointing.values())or just dropping this case. In real data, there will always be a mispointing, so generally we never have it perfectly equal (and of course this check only works if this is mcdata anyhow).
There was a problem hiding this comment.
could we fix this? It shouldn't change anything for parallel simulations, but it would solve #17 for divergent pointing
protopipe/scripts/write_dl1.py
Outdated
|
|
||
| parser.add_argument( | ||
| "--array_direction_altaz", type=tuple, default=None, | ||
| help="set an array pointing direction. Required for non parallel pointing" |
There was a problem hiding this comment.
I wonder if it's not better to just compute the array direction as the barycenter of the telescope pointings? Then it's automatic and works for both mis-pointing and divergent pointing.
There was a problem hiding this comment.
The desired array direction could be anything, not necessarily the barycenter of the actual telescopes pointings.
There was a problem hiding this comment.
However, I can add it as an option? 'barycenter' could be passed for this argument.
There was a problem hiding this comment.
It's probably a good "default" behavior at least, so the user doesn't have to choose it themselves. I think for divergent pointing, the optimal will always be the barycenter, though, right? At least if you want a roughly circular FOV
There was a problem hiding this comment.
This CL arguments change has to be replicated also to write_dl2.py
There was a problem hiding this comment.
Ah, and could you also update the docs accordingly?
You just need to copy-paste the new --help output in these 2 places,
protopipe/docs/scripts/data_training.rst
protopipe/docs/scripts/DL2.rst
HealthyPear
added
pipeline applications
|
What is the status of this PR @vuillaut ? It seems that some details are to be specified, am I right? |
protopipe/scripts/write_dl1.py
Outdated
|
|
||
| parser.add_argument( | ||
| "--array_direction_altaz", type=tuple, default=None, | ||
| help="set an array pointing direction. Required for non parallel pointing" |
There was a problem hiding this comment.
This CL arguments change has to be replicated also to write_dl2.py
protopipe/pipeline/event_preparer.py
Outdated
| # pointing to predict | ||
| for tel_id in tels_pointing: | ||
| if tels_pointing[tel_id].alt != event.mcheader.run_array_direction[1]: | ||
| tels_pointing = None |
There was a problem hiding this comment.
could we fix this? It shouldn't change anything for parallel simulations, but it would solve #17 for divergent pointing
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
=========================================
- Coverage 0.39% 0.39% -0.01%
=========================================
Files 20 20
Lines 2268 2279 +11
=========================================
Hits 9 9
- Misses 2259 2270 +11
Continue to review full report at Codecov.
|
…thub_CI Enable CI from GitHub actions
…pdate_TRAINING_integration_test_diffuse Update training integration tests
…integration_test_pipeline_up_to_models
…ve_build_models_and_mva Small improvements to modeling script
…ntegration_test_pipeline_up_to_models Setup of pipeline integration testing up to modeling
…on-tests Fix pipeline integration test workflow
Small change to better handle non-parallel array pointing.
(0,pi/2))