TelescopeParameterLookup does not check wether telescope is in attaached subarray

[nbiederbeck]

The following code fails:

TelescopeParamtereLookup.tel[0]
*** KeyError: "TelescopeParameterLookup: no parameter value was set for telescope with tel_id=0. Please set it explicitly, or by telescope type or '*'."

which is ok, because it was really not set by '*', because it is not in the attached subarray.

The error message could be improved though, because 0 is a nonsenical telescope id.

I would prefer that for those cases the error message would be

KeyError: "TelescopeParameterLookup: telescope with tel_id=0 is not in attached subarray.

ctapipe/ctapipe/core/telescope_component.py

Lines 208 to 219 in 2021ad1

	if isinstance(tel, (int, np.integer)):
	try:
	return self._value_for_tel_id[tel]
	except KeyError:
	raise KeyError(
	f"TelescopeParameterLookup: no "
	f"parameter value was set for telescope with tel_id="
	f"{tel}. Please set it explicitly, "
	f"or by telescope type or '*'."
	)

[Tobychev]

As I don't know of a way of getting a KeyError from a dict than asking for a missing key, I agree that having the error message say anything but "telescope with id {id} not found in provided subarray" is confusing.

But are you also asking for a check before the try catch that handles the special case tel=0?

[maxnoe]

0 is a special case in that we basically always know that it is an invalid tel_id, however, with our subselected arrays, any tel id might actually be not in the subarray

[nbiederbeck]

[...] might actually be not in the subarray

Yes and I'm asking for a distinction of these two cases, 1) not having set a parameter value and 2) asking for a telescope that is not there.

This came up during debugging, I think it might actually not be a problem in normal operations, since we just loop over all telescopes in a subarray (that then are there by definition)?

[maxnoe]

Yes, that is the right thing to do, however, I was arguing that we shouldn't just treat 0 specially, but we should check.

I would suggest to check only in in the except block if the telescope is in the subarray, as that would be expensive otherwise.

E.g.:

try:
    return self._value_for_tel_id[tel]
except KeyError:
    if tel not in self.subarray.tel:
        raise KeyError(f"Unknown telescope id: {tel_id}")
    else:
        raise KeyError("No value configured vor telescope id: {tel_id}")

Maybe introduce new exception classes for the two cases, i.e. MissingConfigValue and UnknownTelescope

[maxnoe]

The code to be changed in the way described above is here:

ctapipe/ctapipe/core/telescope_component.py

Lines 216 to 225 in a987cc8

	if isinstance(tel, (int, np.integer)):
	try:
	return self._value_for_tel_id[tel]
	except KeyError:
	raise KeyError(
	f"TelescopeParameterLookup: no "
	f"parameter value was set for telescope with tel_id="
	f"{tel}. Please set it explicitly, "
	f"or by telescope type or '*'."
	)