DL1 checks fix and updates #881

moralejo · 2022-01-21T13:45:26Z

Solved issue with non-updating histograms when moving the run number slider
extend run slider bar in case of more than 300 runs
added RA and dec info in dl1 data check tables to allow selection by sky region
added pixel-wise counting of nearby bright stars, and use that info for excluding some pixels/subruns from the calculations of averages, so that we avoid mistaking them for e.g. noisy pixels

codecov · 2022-01-21T14:00:31Z

Codecov Report

Merging #881 (a990c7b) into master (b22332d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #881   +/-   ##
=======================================
  Coverage   83.83%   83.83%           
=======================================
  Files          77       77           
  Lines        5968     5968           
=======================================
  Hits         5003     5003           
  Misses        965      965

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 b22332d...a990c7b. Read the comment docs.

lstchain/datachecks/containers.py

setup.py

moralejo · 2022-01-24T10:49:32Z

from lstchain.reco.utils import location should be ok, right? No need to use EarthLocation explicitly

maxnoe · 2022-01-24T11:00:44Z

from lstchain.reco.utils import location should be ok, right? No need to use EarthLocation explicitly

Yes, that is the correct position for LST-1

Extend run slider bar in case of more than 300 runs

…ulation

…de up...

maxnoe · 2022-01-28T17:29:59Z

docs/lstchain_api/scripts/index.rst

+
+Usage
+-----
+.. argparse::


maxnoe · 2022-01-28T17:31:54Z

lstchain/datachecks/containers.py

@@ -35,6 +40,8 @@ class DL1DataCheckContainer(Container):
    num_ucts_jumps = Field(-1, 'Number of observed (and corrected) UCTS jumps')
    mean_alt_tel = Field(None, 'Mean telescope altitude')
    mean_az_tel = Field(None, 'Mean telescope azimuth')
+    tel_ra = Field(None, 'Telescope pointing RA (deg)')
+    tel_dec = Field(None, 'Telescope pointing declination')


container fields should have the correct defaults, so that if a default container is written, the correct tables are setup. None will result in the column being skipped. I think these (and the above) should be np.nan * u.deg and ucts_trigger_type and trigger_type should probably be 0 (they are bitfields of which at least one bit must be set to be a valid trigger type, so 0 would make sense as invalid value marker.

I comment here because I cannot comment on the old code. But to setup a logger related to ctapipe.instrument.camera in a container class definition is really strange and should probably be (re)moved.

Having this in the class body get's executed at import time. So this will set it for the whole of lstchain, as soon as lstchain/datachecks/container is imported. If you need this for some reason, I guess connected to cta-observatory/ctapipe#1828, you should set this in the main method of the script, not here.

I comment here because I cannot comment on the old code. But to setup a logger related to ctapipe.instrument.camera in a container class definition is really strange and should probably be (re)moved.

~~I don't remember why I put that there in the first place... gone!~~

I "remembered":

Coordinate (nan m, nan m) lies outside camera Coordinate (nan m, nan m) lies outside camera Coordinate (nan m, nan m) lies outside camera Coordinate (nan m, nan m) lies outside camera Coordinate (nan m, nan m) lies outside camera .... etc etc

In this way I don't get a warning when I try to get the pixel id which corresponds to certain coordinates, some of which turn out to be nans (I know there would be other solutions, but this one is simple).

Having this in the class body get's executed at import time. So this will set it for the whole of lstchain, as soon as lstchain/datachecks/container is imported. If you need this for some reason, I guess connected to cta-observatory/ctapipe#1828, you should set this in the main method of the script, not here.

Ok, understood. I moved the geometry warning suppressing to the beginning of the script lstchain_check_dl1.py

This also made me realize I was still projecting wrongly some rare events outside the camera (=> index -1) onto the last pixel, so I fixed that also.

This logging setup

maxnoe · 2022-01-28T17:34:41Z

lstchain/datachecks/containers.py

+        time_utc = Time(table['dragon_time'][int(len(table)/2)],
+                        format="unix", scale="utc")
+        # Calculate telescope pointing in sky coordinates
+        telescope_pointing = SkyCoord(alt=self.mean_alt_tel*u.rad,


Why do self.mean_alt_tel / self.mean_az_tel not already have units?

maxnoe · 2022-01-28T17:34:52Z

lstchain/datachecks/containers.py

+                                      az=self.mean_az_tel*u.rad,
+                                      frame=AltAz(obstime=time_utc,
+                                                  location=location))
+        self.tel_ra = telescope_pointing.icrs.ra.to(u.deg).value


to_value(u.deg)

Why strip the unit away at all?

Also, this does the altaz → icrs transformation twice I believe. Transform once, than access the result.

I would probably only store self.pointing_icrs = self.pointing_altaz.transform_to('icrs') and not the individual values without units.

lstchain/datachecks/containers.py

maxnoe · 2022-01-28T17:38:05Z

lstchain/datachecks/containers.py

+        obstime = Time(sampled_times[int(len(sampled_times)/2)],
+                       scale='utc', format='unix')
+        horizon_frame = AltAz(location=location, obstime=obstime)
+        pointing = SkyCoord(az=self.mean_az_tel*u.rad,


See? just store the coordinate as astropy coordinate above and you don't have to reconstruct the same object here.

lstchain/datachecks/containers.py

maxnoe · 2022-01-28T17:44:37Z

lstchain/scripts/lstchain_longterm_dl1_check.py

-                  # currently (as of lstchain 0.5.3) event numbers are post-cleaning!:
+                  'min_azimuth': [],
+                  'max_azimuth': [],
+                  'mean_azimuth': [],


Maybe nothing to change now, but this approach of dict of lists seems rather clumsy as you have multiple locations where you have to update if you add a new column.

I think a list of dicts would be much easier to maintain:

subrunwise_data = [] for subrun in subruns: data = {} data['charge_mean'] = .... subrunwise_data.append(data)

Like this, you only have a single place and much less to setup and pandas and astropy tables understand the format the same (its the difference between DataFrame.to_dict() and DataFrame.to_dict(orient='records') in pandas.

lstchain/scripts/lstchain_longterm_dl1_check.py

Solved projection of (rare) image centroids outside the camera onto the last pixel

…t arrays

…e pandas dataframes by astropy tables

moralejo · 2022-01-31T19:05:20Z

@maxnoe I think your suggestions are now implemented, and I have tested this with a few hours of DL1 files.

maxnoe

Let's merge this as is now.

We should break this up into manageable and reviewable parts, but that can be done at a later stage together with the rest of lstchain.

moralejo · 2022-02-01T10:05:13Z

Let's merge this as is now.

Ok, thanks!

We should break this up into manageable and reviewable parts, but that can be done at a later stage together with the rest of lstchain.

Absolutely... but at least this PR, besides adding quite some functionality, goes in the right direction of doing some simplifications (e.g. use of astropy tables).

moralejo requested a review from rlopezcoto January 21, 2022 13:45

moralejo marked this pull request as draft January 21, 2022 16:17

maxnoe reviewed Jan 21, 2022

View reviewed changes

lstchain/datachecks/containers.py Outdated Show resolved Hide resolved

moralejo marked this pull request as ready for review January 21, 2022 18:07

moralejo marked this pull request as draft January 21, 2022 18:30

moralejo marked this pull request as ready for review January 21, 2022 19:28

moralejo marked this pull request as draft January 23, 2022 19:21

moralejo changed the title ~~Solved issue with non-updating histograms~~ DL1 checks fix and updates Jan 23, 2022

moralejo mentioned this pull request Jan 24, 2022

Per-dataset data check summary #873

Open

maxnoe reviewed Jan 24, 2022

View reviewed changes

lstchain/datachecks/containers.py Show resolved Hide resolved

maxnoe reviewed Jan 24, 2022

View reviewed changes

setup.py Outdated Show resolved Hide resolved

moralejo force-pushed the update_dl1checks branch from c951780 to eb710df Compare January 24, 2022 16:49

moralejo added 8 commits January 24, 2022 18:03

Solved issue with non-updating histograms

5f79225

Extend run slider bar in case of more than 300 runs

Pointing calculations

a002b2e

Removed unused imports

cace4a0

Added RA and dec to DL1 checks tables

50f5d4e

typo

bd46cb7

solve pandas warning

6265dfa

New attempt at getting rid of pandas warning

b27ff5e

More clear code in the ucts jumps counting

987b95f

morcuended mentioned this pull request Jan 24, 2022

Change name of command for longterm lstchain script cta-observatory/lstosa#88

Closed

moralejo added 6 commits January 24, 2022 18:07

Added ra, dec to runsummary table, also made proper mean azimuth calc…

e0ac45d

…ulation

Make the omit-pdf option work in all operation modes

a875c29

Added longterm_dl1_check.py to entry points

f1eb3ab

Use the right numpy function's name (arctan2) instead of the one I ma…

bc27c33

…de up...

Per-subrun pixel-wise counting of nearby bright stars

4f657ed

Fixed issue with subrun time

ba3f1c6

moralejo marked this pull request as ready for review January 28, 2022 09:45

Merge remote-tracking branch 'upstream/master' into update_dl1checks

229ef75

maxnoe reviewed Jan 28, 2022

View reviewed changes

docs/lstchain_api/scripts/index.rst

Usage

-----

.. argparse::

Copy link

Member

maxnoe Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL! nice

maxnoe reviewed Jan 28, 2022

View reviewed changes

lstchain/datachecks/containers.py Outdated Show resolved Hide resolved

maxnoe reviewed Jan 28, 2022

View reviewed changes

lstchain/datachecks/containers.py Outdated Show resolved Hide resolved

maxnoe reviewed Jan 28, 2022

View reviewed changes

lstchain/datachecks/containers.py Outdated Show resolved Hide resolved

maxnoe reviewed Jan 28, 2022

View reviewed changes

lstchain/scripts/lstchain_longterm_dl1_check.py Outdated Show resolved Hide resolved

moralejo added 11 commits January 28, 2022 21:06

Addressed a few of the comments by @maxnoe

cc9536d

Solved projection of (rare) image centroids outside the camera onto the last pixel

Removed unused import

39e2b5a

Solved issue with position_to_pix_index yielding error for empty inpu…

868a36b

…t arrays

z-axis of image centroid distribution converted to rate

e35461a

Wrong time array used for normalization of centroids distribution

656bef5

correct normalization of >200 pe centroid distribution

cefda31

Addressed more of @maxnoe's suggestions about units etc. Replaced som…

2a6c0a7

…e pandas dataframes by astropy tables

Removed unnecessary comment

8ddabb0

Removed None in defaults of class members

3e3398e

Merge remote-tracking branch 'upstream/master' into update_dl1checks

24f017b

Put back None defaults in array data members

a990c7b

moralejo requested a review from maxnoe February 1, 2022 09:47

maxnoe approved these changes Feb 1, 2022

View reviewed changes

moralejo merged commit 9fa55dd into master Feb 1, 2022

moralejo deleted the update_dl1checks branch February 1, 2022 10:05

morcuended mentioned this pull request Feb 1, 2022

Change name of the longterm script cta-observatory/lstosa#98

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DL1 checks fix and updates #881

DL1 checks fix and updates #881

moralejo commented Jan 21, 2022 •

edited

Loading

codecov bot commented Jan 21, 2022 •

edited

Loading

moralejo commented Jan 24, 2022

maxnoe commented Jan 24, 2022

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

moralejo Jan 28, 2022 •

edited

Loading

moralejo Jan 28, 2022 •

edited

Loading

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

maxnoe Jan 28, 2022

moralejo commented Jan 31, 2022

maxnoe left a comment

moralejo commented Feb 1, 2022

DL1 checks fix and updates #881

DL1 checks fix and updates #881

Conversation

moralejo commented Jan 21, 2022 • edited Loading

codecov bot commented Jan 21, 2022 • edited Loading

Codecov Report

moralejo commented Jan 24, 2022

maxnoe commented Jan 24, 2022

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

moralejo Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

moralejo Jan 28, 2022 • 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

moralejo commented Jan 31, 2022

maxnoe left a comment

Choose a reason for hiding this comment

moralejo commented Feb 1, 2022

moralejo commented Jan 21, 2022 •

edited

Loading

codecov bot commented Jan 21, 2022 •

edited

Loading

moralejo Jan 28, 2022 •

edited

Loading

moralejo Jan 28, 2022 •

edited

Loading