Skip to content
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

Upgrade calibration benchmarks #59

Merged

Conversation

HealthyPear
Copy link
Member

@HealthyPear HealthyPear commented Jul 30, 2020

Description:

  • now we don't need protopipe for this part, we can use directly ctapipe-stage1-process with proper configuration to mimic CTA-MARS
  • added a new method to find optimized cleaning thresholds which substitutes the old one
  • all Prod3b cameras are now supported (but some, like e.g. FlashCam, not optimized)

TO DO

@HealthyPear HealthyPear added documentation Documentation or services hosting it enhancement New feature or request benchmarks labels Jul 30, 2020
@HealthyPear HealthyPear added this to the Release 0.3 milestone Jul 30, 2020
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@HealthyPear HealthyPear added this to In progress in Pipeline features and enhancements via automation Jul 30, 2020
@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #59 (d4d31bb) into master (3225536) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #59   +/-   ##
=======================================
  Coverage   31.66%   31.66%           
=======================================
  Files          20       20           
  Lines        2201     2201           
=======================================
  Hits          697      697           
  Misses       1504     1504           

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 3225536...d4d31bb. Read the comment docs.

@HealthyPear HealthyPear marked this pull request as ready for review December 1, 2020 10:15
@HealthyPear
Copy link
Member Author

I might have mistakenly removed an image cleaning benchmark, but it was anyway an older version with respect the one contained in PR #76 .

Please, until PR #62 don't mind the readthedocs check as it is broken at this stage (and is anyway only a nice thing I wanted to add as a backup for the docs).

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

kosack commented on 2020-12-01T11:18:09Z
----------------------------------------------------------------

For accessing tables in DL1 files, you should now use read_table (which reads and returns an astropy Table directly, no need for extra conversion, and units are handled automatically:

from ctapipe.io import read_table

all_mc_images = read_table(filename, "/simulation/event/telescope/images")

For loading all the telescope tables, it's also easy, since you can load the subarray first:

subarray = SubarrayDescription.from_hdf(filename)

for tel_id in subarray.tel_ids:

tab = read_table(filename, f"/dl1/event/telescope/images/tel_{tel_id:03d}")


@HealthyPear HealthyPear mentioned this pull request Dec 1, 2020
2 tasks
Pipeline features and enhancements automation moved this from In progress to Review in progress Dec 4, 2020
Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

a few comments in general:

Binary files

should not really include binary files in the repo, rather download them from elsewhere. (like the ROOT files), not sure if those are allowed to be exposed publicly anyhow. Might be a good reason to move this to GitLab and not public.

In benchmarks_DL1_calibration.ipynb and elsewhere

The suffix is not used in this function, it's just returned as is, so redundant

def load_reset_simtel(indir = "./", fileName = "gamma_20deg_180deg_run100___cta-prod3-demo-2147m-LaPalma-baseline.simtel.gz", max_events=None, config="test"):
    """(Re)load the simtel file for all events and telescopes."""
    source = event_source(input_url=f"{indir}/{fileName}", max_events=max_events)
    suffix = config # all generated plots will have this as a suffix in their name
    return source, suffix

It would be better to drop this function and use with statements when you use the event source later, so that files are correctly closed. E.g.:

with event_source(f"{indir}/{filename}") as source:
    # do stuff...

even better and less error prone, store indir and filename as pathlib.Path objects, so you can do event_source(indir / filename) and it will work on both linux and windows. You can just turn them into paths using indir = pathlib.Path(indir).

tables.open is never closed

In many places you use tables.open, but never close the file after (leading to memory and file handle leaks). Better to use with again, or just use ctapipe.io.read_table to get tables, which handles it internally

from ctapipe.io import read_table
filepath = Path(indir) / filename
true_images = read_table(filepath, f"/simulation/event/telescope/images/{tel}")
reco_images = read_table(filepath, f"/dl1/event/telescope/images/{tel}")

# or if you don't want to open/close twice, you can do:
with tables.open_file(filepath) as h5file:
    true_images = read_table(h5file, f"/simulation/event/telescope/images/{tel}")
    reco_images = read_table(h5file, f"/dl1/event/telescope/images/{tel}")

Note you then get back Astropy QTables, so would need to change a bit the code using them, but in general it will simplify that code (reco_images['image'] instead of reco_images.col('image')), and no more need to do true_images = Table(trueimages[:]) since that is done for you already

Also, it may be easier to read to explicitly show the table name to tel_id relationship:

true_images = read_table(h5file, f"/simulation/event/telescope/images/tel_{tel_id:03d}")

Pipeline features and enhancements automation moved this from Review in progress to Reviewer approved Dec 7, 2020
@HealthyPear HealthyPear merged commit e1239d0 into cta-observatory:master Dec 7, 2020
Pipeline features and enhancements automation moved this from Reviewer approved to Done Dec 7, 2020
@HealthyPear HealthyPear deleted the upgrade-benchmarks_calibration branch December 7, 2020 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks documentation Documentation or services hosting it enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants