-
Notifications
You must be signed in to change notification settings - Fork 267
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
Irf maker and cut optimiser #2473
base: main
Are you sure you want to change the base?
Conversation
ctapipe/irf/irf_classes.py
Outdated
help="Percent of events in each energy bin keep after the theta cut", | ||
).tag(config=True) | ||
|
||
def calculate_theta_cuts(self, theta, reco_energy, energy_bins): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Classes that basically are configurable functions (i.e. have traitlets and then a single method) should use __call__
I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After recently having tried to explain how __call__
means you can do object(args) to a beginner, I think that syntax has too much magic and explicit is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more important to be consistent. If that's a decision we should make, we should make it everywhere and not just here. However, regarding this particular point, I have a different opinion.
We have many instances in ctapipe now that implement __call__
and I think it's the correct thing to do.
Beginners have problems with many concepts, that shouldn't mean that we optimize our code for being understandable by beginners vs. writing idiomatic python.
ctapipe/irf/optimise.py
Outdated
def __init__(self, gh_cuts=None, offset_lim=None): | ||
self.gh_cuts = gh_cuts | ||
if gh_cuts: | ||
self.gh_cuts.meta["extname"] = "GH_CUTS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extname is the name of a FITS extension. You should't set this in the metadata of the table, but only when writing out, it's the name
argument of write
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got
TypeError: write_table_fits() got an unexpected keyword argument 'name'
when I tried self.gh_cuts.write(out_name, name="GH_CUTS", format="fits", overwrite=overwrite)
and I don't find anything about a name
argument to table.write
, do you mean I should manage the fits file more directly and just cast the tables to hdus?
ctapipe/irf/select.py
Outdated
|
||
quality_criteria = List( | ||
default_value=[ | ||
("multiplicity 4", "np.count_nonzero(tels_with_trigger,axis=1) >= 4"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want a multiplicity cut of 4 by default? I would remove that and only require the valid reconstruction.
ctapipe/tools/make_irf.py
Outdated
self.source_offset_bins = self.bins.source_offset_bins() | ||
self.fov_offset_bins = self.opt_result.offset_lim | ||
|
||
self.particles = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protons and electrons should probably be optional or not given at all.
You only need them to compute a background model, which we maybe should split into a separate tool (ctapipe-compute-background-from-simulations
) or similar, since we won't use simulations for computing the background component later.
ctapipe/tools/make_irf.py
Outdated
) | ||
) | ||
|
||
# calculate sensitivity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
computing sensitivity should be separate from computing IRFs I think.
In datapipe, we will need to compute the signal-related IRFs from the planned tailored simulations, but won't have proton, electron or heavier nuclei.
ctapipe/tools/make_irf.py
Outdated
) | ||
# Here we use reconstructed energy instead of true energy for the sake of | ||
# current pipelines comparisons | ||
bias_resolution = energy_bias_resolution( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bias_resolution are benchmarks that can be computed from events (like here) or from the IRFs (functions in pyirf or methods of the irf objects in gammapy), I'd not compute them here in the IRF tool.
ctapipe/tools/make_irf.py
Outdated
|
||
# Here we use reconstructed energy instead of true energy for the sake of | ||
# current pipelines comparisons | ||
ang_res = angular_resolution( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, it's a benchmark, not an IRF, so I'd say it doesn't belong into the IRF tool
643a841
to
7c2efc7
Compare
…egression is fixed
36dbfb0
to
df47f71
Compare
As we are planning to produce IRFs on events that were not used to select the GH cut, it makes sense to split the procedure generating IRFs into two tools. Because this splitting results in a large restructuring of the the previous PR on generating IRFs (#2315) that was basically working properly, I open a new PR to preserve the discussion on the old attempt and let its more or less functional code serve as easy reference.