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

Refactor initialization of telescope parameters, move to own module #2190

Merged
merged 2 commits into from
Jan 13, 2023

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Jan 6, 2023

I came across code I didn't first understand in the initialization of the TelescopeParameter traits, trying to improve the code, I ran into a circular import problem (as the straight forward code would need to check isinstance(trait, TelescopeParameter and TelescopeComponent is in a module imported by ctapipe.core.traits).

So the best solution was to move the TelescopeComponent / TelescopeParameter related things into their own submodule ctapipe.core.telescope_component.

I think this is an improvement.

The main change (aside from moving things around) is replacing this code:

# configure all of the TelescopeParameters
for trait in list(self.class_traits()):
    try:
        getattr(self, trait).attach_subarray(subarray)
    except (AttributeError, TypeError):
        pass

with

# configure all of the TelescopeParameters
for attr, trait in self.class_traits().items():
    if not isinstance(trait, TelescopeParameter):
        continue

    # trait is the TelescopeParameter descriptor at the class,
    # need to get the value at the instance, which will be a TelescopePatternList
    pattern_list = getattr(self, attr)
    if pattern_list is not None:
        pattern_list.attach_subarray(subarray)

which is I think much clearer.

This also has the additional benefit of not silently ignoring errors that could potentially happen in attach_subarray.

@maxnoe maxnoe force-pushed the telescope_component_refactor branch from 3d96716 to a9481dd Compare January 6, 2023 14:13
@maxnoe maxnoe requested review from LukasNickel, kosack and nbiederbeck and removed request for LukasNickel and kosack January 9, 2023 09:06

Returns
-------
instace
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
instace
Instance

looked up by tel_id. This must be done before using the ``.tel[x]`` property
"""
self._subarray = subarray
self._lookup.attach_subarray(subarray)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work when in init self._lookup = None?

Copy link
Contributor

@nbiederbeck nbiederbeck Jan 9, 2023

Choose a reason for hiding this comment

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

I assume this does not happen in "normal operations" but we might type-hint in __init__ anyways

ctapipe/core/telescope_component.py Show resolved Hide resolved
default_value = self.validate(self, default_value)

if "help" not in kwargs:
self.help = "A TelescopeParameter"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should raise an exception, to have everything documented?

ctapipe/core/tests/test_telescope_component.py Outdated Show resolved Hide resolved
default_value=[
("type", "*", 10.0),
("type", "LST_LST_LSTCam", 100.0),
("type", "*", 200.0), # should overwrite everything with 200.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
("type", "*", 200.0), # should overwrite everything with 200.0
("type", "*", 200.0), # should overwrite everything above with 200.0

nbiederbeck
nbiederbeck previously approved these changes Jan 9, 2023
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.

yes, this is much easier to follow. I didn't like relying on an exception to type-check

@maxnoe maxnoe merged commit 636ee75 into master Jan 13, 2023
@maxnoe maxnoe deleted the telescope_component_refactor branch January 13, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants