Fix method camera_to_sky for real events #421

JouvinLea · 2020-05-26T19:31:44Z

This PR fix the problem in the method computing camera_to_sky for event to take properly into account the time of the observation in the computation.

For the moment, just use time if provided and the default obs time in utils if not printing a warning.

I think the solution to use HA/DEC for MC event instead of RA/DEC should be done in another method when you compute ra/dec for those SkyCoords. For example, if I read correctly to compute the HA from RA, we need to extract the sideral time from the obs time and the location.

scale_time = sky_coords.obstime.scale
longitude = sky_coords.location.longitude
latitude = sky_coords.location.latitude
time=Time(obstime, scale, location=(longitude, latitude)
HA = sky_coords.icrs.ra.to("hourangle") -time.sidereal_time("apparent")
dec = sky_coords.icrs.dec

Also I need to improve the test to add a check on a ra/dec value when we add the time.

codecov · 2020-05-26T19:43:22Z

Codecov Report

Merging #421 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
+ Coverage   41.46%   41.53%   +0.07%     
==========================================
  Files          76       76              
  Lines        6179     6187       +8     
==========================================
+ Hits         2562     2570       +8     
  Misses       3617     3617

Impacted Files	Coverage Δ
lstchain/reco/tests/test_utils.py	`100.00% <100.00%> (ø)`
lstchain/reco/utils.py	`68.93% <100.00%> (+0.97%)`	⬆️
lstchain/tests/test_lstchain.py	`100.00% <100.00%> (ø)`

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 3995ebb...0668c90. Read the comment docs.

rlopezcoto · 2020-05-27T06:09:24Z

lstchain/reco/utils.py

@@ -203,16 +203,24 @@ def reco_source_position_sky(cog_x, cog_y, disp_dx, disp_dy, focal_length, point
    return camera_to_sky(src_x, src_y, focal_length, pointing_alt, pointing_az)


-def camera_to_sky(pos_x, pos_y, focal, pointing_alt, pointing_az):
+def camera_to_sky(pos_x, pos_y, focal, pointing_alt, pointing_az, time = None):


maybe we should change the name of the function. As @maxnoe pointed out in #420 , this method is converting to a HorizonFrame

maxnoe · 2020-05-27T06:35:42Z

lstchain/reco/utils.py

+        horizon_frame = AltAz(location=location, obstime=time)
+    else:
+        horizon_frame = AltAz(location=location, obstime=obstime)
+        print("No time given, use default observation time. To be use only for MC data.")


Do not use print here, if at all, use logging. But It is unnecessary here.

maxnoe · 2020-05-27T08:42:39Z

lstchain/reco/utils.py



-def camera_to_sky(pos_x, pos_y, focal, pointing_alt, pointing_az, time = None):
+def camera_to_altaz_frame(pos_x, pos_y, focal, pointing_alt, pointing_az, time = None):


The frame in the end is a little strange (Either its camera_frame_to_altaz_frame or camera_to_altaz).
I would just leave out the frame.

time should be obstime to be consistent with the astropy API, and then you also don't need the if statement below.

Thanks @maxnoe for your usefull input.
Here I disagree with you but may I misunderstood. I think for MC events we need to use the obstime_mc in order to be able to create a posteriori a ra/dec for MC event in HA/dec as proposed @moralejo . To perform that, I understood we will need:

longitude = sky_coords.location.longitude latitude = sky_coords.location.latitude time=Time(obstime, scale, location=(longitude, latitude) HA = sky_coords.icrs.ra.to("hourangle") -time.sidereal_time("apparent") dec = sky_coords.icrs.dec

So we need to have a false obstime to be able to then transform to HA.

I don't think @moralejo was proposing to actually calculate HA, just mentioning the possibility.

At the moment, there is no usecase I know of for having simulations in another frame than horizon.

I think that the monte carlo processing should be fully agnostic of any obstime. Introducing a fake obstime is a hack (that should be unnecessary).

If it at some position is necessary to introduce a fake time, I would be glad to know, because that means we nead it fixed.

maxnoe · 2020-05-27T08:43:57Z

lstchain/reco/utils.py


    """
+
+    if time:


This if is not needed. Just use obstime=obstime and it works fine also in the case where obstime is None

maxnoe · 2020-05-27T08:45:22Z

lstchain/reco/utils.py


    Returns
    -------
-    sky frame: `astropy.coordinates.sky_coordinate.SkyCoord`
-
+    sky frame: `astropy.coordinates.AltAz`


Actually the return value is a astropy.coordinates.SkyCoordinate that has a frame of AltAz.

So maybe write `astropy.coordinates.SkyCoord` in AltAz frame`

maxnoe · 2020-05-27T08:45:56Z

lstchain/reco/utils.py

+        pointing altitude in angle unit
+    pointing_az: `~astropy.units.Quantity`
+        pointing altitude in angle unit
+    time: `~astropy.time.Time`


Call it obstime

maxnoe · 2020-05-27T10:32:27Z

lstchain/reco/utils.py

@@ -43,8 +44,8 @@

 # position of the LST1
 location = EarthLocation.from_geodetic(-17.89139 * u.deg, 28.76139 * u.deg, 2184 * u.m)
-obstime = Time('2018-11-01T02:00')
-horizon_frame = AltAz(location=location, obstime=obstime)
+obstime_mc = Time('2018-11-01T02:00')


Just remove this.

Why was this added anyway? What is so special about 2018-11-01T02:00 that it shoud be used as time fo mc?

maxnoe · 2020-05-27T10:32:53Z

lstchain/reco/utils.py

+    if obstime:
+        horizon_frame = AltAz(location=location, obstime=obstime)
+    else:
+        horizon_frame = AltAz(location=location, obstime=obstime_mc)


Again: just don't use an obstime for mc please. It has no meaning.

maxnoe · 2020-05-27T10:56:55Z

lstchain/reco/utils.py

-        horizon_frame = AltAz(location=location, obstime=obstime)
-    else:
-        horizon_frame = AltAz(location=location, obstime=obstime_mc)
+    if not obstime:
        logging.warning("No time given, use default observation time. To be use only for MC data.")


Now, you don't use any obstime (obstime is None, you are not using the global)

yes this is what you ask no?

But the logging message says something about a default obstime, you not using any obstime

And I would say, as this is the expected situation for every simulated event, this should not be a "warning".

Warnings should be for unexpected conditions that require the user to do something. Meaningless warnings are dangerous because then users start ignoring them and potentially start ignoring also the important warnings.

This is because you are too fast, I already modify the print in the time you did your comment

I agree for the warning

maxnoe · 2020-05-27T10:59:57Z

Btw. the frame to get local hourangle and declination would be the Celestial Intermediate Reference System:
https://docs.astropy.org/en/stable/api/astropy.coordinates.CIRS.html

EDIT: now, the origin of the CIRS is the CIP, so not yet a local coordinate frame.

maxnoe · 2020-05-27T11:25:55Z

There is an open issue for requesting the local equatorial frame in astropy here:
astropy/astropy#10239

rlopezcoto

@JouvinLea is this PR ready (all comments taken into account, except this small one I just spotted in one comment)?
Are you also planning to implement the writing of RA/Dec coordinates in the DL1/2 dataframe?

rlopezcoto · 2020-06-02T10:17:33Z

lstchain/reco/utils.py

@@ -227,9 +236,13 @@ def camera_to_sky(pos_x, pos_y, focal, pointing_alt, pointing_az):
    focal = 28*u.m
    pointing_alt = np.array([1.0, 1.0]) * u.rad
    pointing_az = np.array([0.2, 0.5]) * u.rad
-    sky_coords = utils.camera_to_sky(pos_x, pos_y, focal, pointing_alt, pointing_az)
+    sky_coords = utils.camera_to_altaz_frame(pos_x, pos_y, focal, pointing_alt, pointing_az)


camera_to_altaz_frame -> camera_to_altaz

maxnoe · 2020-06-02T10:59:16Z

Is the obstime defined globally in the utils.py file used somewhere else? It should be removed.

JouvinLea · 2020-06-03T05:57:11Z

if you think it can be merge, I am personally done.
otherwise tell me what I should change.

Obstime at the beginning of the files is used to define the horizon_frame used in other methods . And I will not do this clean up in this PR. maybe one PR can be done to clean the utils.py in the future.

Once this is merge, I will do another PR for implementing ra/dec in the DL2 dataframe but other PR is better because this is another subject.

rlopezcoto

Ok, I agree with @JouvinLea and we should move on with this. @maxnoe could you open an issue on the cleanup of utils.py according to what we discussed here?

addressed

fix camera to sky for event

6db0bce

rlopezcoto reviewed May 27, 2020

View reviewed changes

fix name method

3501cfd

maxnoe previously requested changes May 27, 2020

View reviewed changes

fix minor docs

f967c50

maxnoe reviewed May 27, 2020

View reviewed changes

remove obstime for mc

de81a16

JouvinLea force-pushed the fix_camera_to_Sky branch from 90210cd to de81a16 Compare May 27, 2020 10:58

change log warning to log info

f91dc94

rlopezcoto reviewed Jun 2, 2020

View reviewed changes

fix docs

54d5147

rlopezcoto approved these changes Jun 3, 2020

View reviewed changes

Merge branch 'master' into fix_camera_to_Sky

0668c90

rlopezcoto merged commit 40e6410 into cta-observatory:master Jun 4, 2020

rlopezcoto mentioned this pull request Jun 19, 2020

introduce radec_to_camera function #437

Merged

This was referenced Aug 27, 2020

Transformation of camera to Sky coordinates works only for MC #420

Closed

Speeding up coordinate transformation and writing them in the DL2 files #501

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix method camera_to_sky for real events #421

Fix method camera_to_sky for real events #421

JouvinLea commented May 26, 2020 •

edited

Loading

codecov bot commented May 26, 2020 •

edited

Loading

rlopezcoto May 27, 2020

maxnoe May 27, 2020

maxnoe May 27, 2020

JouvinLea May 27, 2020 •

edited

Loading

maxnoe May 27, 2020 •

edited

Loading

maxnoe May 27, 2020

maxnoe May 27, 2020 •

edited

Loading

maxnoe May 27, 2020

maxnoe May 27, 2020

maxnoe May 27, 2020

maxnoe May 27, 2020

maxnoe May 27, 2020

JouvinLea May 27, 2020

maxnoe May 27, 2020

maxnoe May 27, 2020

JouvinLea May 27, 2020

JouvinLea May 27, 2020

maxnoe commented May 27, 2020 •

edited

Loading

maxnoe commented May 27, 2020

rlopezcoto left a comment

rlopezcoto Jun 2, 2020

maxnoe commented Jun 2, 2020

JouvinLea commented Jun 3, 2020

rlopezcoto left a comment



		def camera_to_sky(pos_x, pos_y, focal, pointing_alt, pointing_az, time = None):
		def camera_to_altaz_frame(pos_x, pos_y, focal, pointing_alt, pointing_az, time = None):

Fix method camera_to_sky for real events #421

Fix method camera_to_sky for real events #421

Conversation

JouvinLea commented May 26, 2020 • edited Loading

codecov bot commented May 26, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JouvinLea May 27, 2020 • edited Loading

Choose a reason for hiding this comment

maxnoe May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxnoe May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxnoe commented May 27, 2020 • edited Loading

maxnoe commented May 27, 2020

rlopezcoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxnoe commented Jun 2, 2020

JouvinLea commented Jun 3, 2020

rlopezcoto left a comment

Choose a reason for hiding this comment

JouvinLea commented May 26, 2020 •

edited

Loading

codecov bot commented May 26, 2020 •

edited

Loading

JouvinLea May 27, 2020 •

edited

Loading

maxnoe May 27, 2020 •

edited

Loading

maxnoe May 27, 2020 •

edited

Loading

maxnoe commented May 27, 2020 •

edited

Loading