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

Remove some obsolete TODOs #2573

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/changes/2573.maintenance.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Removed some obsolete TODOs scattered around the code
8 changes: 1 addition & 7 deletions src/ctapipe/calib/camera/calibrator.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,12 +244,6 @@ def _calibrate_dl1(self, event, tel_id):
waveforms -= np.atleast_2d(dl1_calib.pedestal_offset)[..., np.newaxis]

if n_samples == 1:
# To handle ASTRI and dst
Copy link
Member

Choose a reason for hiding this comment

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

I think this one still applies (see #1777 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we still want to support DSTs? if so, should that be written here as a TODO? I would expect it to be an issue, but nothing in the code.

# TODO: Improved handling of ASTRI and dst
# - dst with custom EventSource?
# - Read into dl1 container directly?
# - Don't do anything if dl1 container already filled
# - Update on SST review decision
dl1 = DL1CameraContainer(
image=np.squeeze(waveforms).astype(np.float32),
peak_time=np.zeros(n_pixels, dtype=np.float32),
Expand Down Expand Up @@ -322,7 +316,7 @@ def __call__(self, event):
event : container
A `~ctapipe.containers.ArrayEventContainer` event container
"""
# TODO: How to handle different calibrations depending on tel_id?

tel = event.r1.tel or event.dl0.tel or event.dl1.tel
for tel_id in tel.keys():
self._calibrate_dl0(event, tel_id)
Expand Down
3 changes: 0 additions & 3 deletions src/ctapipe/core/provenance.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
"""
Provenance-related functionality

TODO: have this register whenever ctapipe is loaded

"""

import json
Expand Down
4 changes: 0 additions & 4 deletions src/ctapipe/image/pixel_likelihood.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@
neg_log_likelihood(image, prediction, spe, ped)
59.9 µs per loop

TODO:
=====
- Need to implement more tests, particularly checking for error states
- Additional terms may be useful to add to the likelihood
"""

import numpy as np
Expand Down
9 changes: 3 additions & 6 deletions src/ctapipe/io/simteleventsource.py
Original file line number Diff line number Diff line change
Expand Up @@ -637,12 +637,9 @@ def prepare_subarray_info(self, telescope_descriptions, header):
)
except ValueError:
telescope = unknown_telescope(mirror_area, n_pixels)

# TODO: switch to warning or even an exception once
# we start relying on this.
self.log.debug(
"Could not determine telescope from sim_telarray metadata,"
" guessing using builtin lookup-table: %d: %s",
self.log.info(
"Could not determine telescope type from sim_telarray metadata,"
" guessing using builtin lookup-table: tel_id %d: %s",
tel_id,
telescope,
)
Expand Down
10 changes: 2 additions & 8 deletions src/ctapipe/reco/hillas_intersection.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
"""

TODO:
- Speed tests, need to be certain the looping on all telescopes is not killing
performance
- Introduce new weighting schemes
- Make intersect_lines code more readable

"""An alternate implementation of the Hillas line-intersection method that more
closely follows the HESS implementation.
"""
import itertools
import warnings
Expand Down
4 changes: 0 additions & 4 deletions src/ctapipe/tools/info.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,6 @@ def _info_tools():
print("the following can be executed by typing ctapipe-<toolname>:")
print("")

# TODO: how to get a one-line description or
# full help text from the docstring or ArgumentParser?
# This is the function names, we want the command-line names
# that are defined in setup.py !???
from textwrap import TextWrapper

from ctapipe.tools.utils import get_all_descriptions
Expand Down
3 changes: 0 additions & 3 deletions src/ctapipe/tools/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ def get_installed_tools():
"""Get list of installed scripts via ``pkg-resources``.

See https://setuptools.pypa.io/en/latest/pkg_resources.html#convenience-api

TODO: not sure if this will be useful ... maybe to check if the list
of installed packages matches the available scripts somehow?
"""
console_tools = {
ep.name: ep.value
Expand Down