Optional altitude #131
Optional altitude #131
Conversation
|
Yes, agreed, providing exactly one of them is better! |
|
Can you resolve the conflicts? |
| @@ -74,18 +74,23 @@ def main(configuration_path, signal_path, predictions_path, disp_model_path, sig | |||
| from ..cta_helpers import horizontal_to_camera_cta_simtel | |||
| source_x, source_y = horizontal_to_camera_cta_simtel( | |||
| az=df[model_config.source_az_column], | |||
| zd=df[model_config.source_zd_column], | |||
| zd=df[model_config.source_zd_column] if model_config.source_zd_column | |||
There was a problem hiding this comment.
You could do this in the preprocessing.py, add a function that fills the zenith into the dataframe.
There was a problem hiding this comment.
Yes.
Was debating that. I really dont know which way I prefer especially with regards to #117.
Both ways feel hacky tbh.
There was a problem hiding this comment.
or do it before the if so that the code is not duplicated.
There was a problem hiding this comment.
You mean one if clause before the function call instead of two clauses in the function call?
Something like
if not config.zd_column:
source_zd = 90 - alt
pointing_zd = 90-pointing_alt
horizontal_to_camera(..., zd, pointing_zd)
Or the other way around for CTA
There was a problem hiding this comment.
See my review comment. Now, I came to think more about it, we should just use altitude for CTA (so change the cta function to take altitude) and convert from zenith to altitude if zenith is given for cta (but there should be no real reason for that to happen).
LukasNickel
force-pushed
the
optional_altitude
branch
from
4c921bd to
4a616ef
Compare
Refers to #123