Improvements to Container classes (#1123)

* change prefix and members of leakage and concentration containers

- Writing `ConcentrationContainers` lead to columns named "concentration_concentration_cog", now removed the redundant prefix
- same for leakage parameters: use "leakage" prefix, and remove that prefix from the names.
- renamed `leakage1_pixel` to `one_pixel_percent` and `leakage1_intensity` to `one_pixel_intensity` to be clearer what they mean

* added ImageParametersContainer to group various parameter containers

* fixed typo

* added EventIndexContainer and TelEventIndexContainers

These contain just basic event header data like tel_id and obs_id

* added telescope type_id to TelIndexContainer

* rename leakage parameters

the old LeakageContainer had the wrong prefix and confusingly named fields. Now a LeakageContainer becomes the following when turned into columns of an output Table:

- leakage_intensity_1pix
- leakage_percent_1pix
- leakage_intensity_2pix
- leakage_percent_2pix

* new containers, renaming, and deprecation

- Added `DeprecatedField`  class to be able to describe fields that will disappear soon
- Introduce `EventIndexContainer` and `TelEventIndexContainer` to store event  index info (event_id, tel_id, obs_id), and deprecated places where these were used elsewhere
- added a MorphologyContainer for morphology parameters (calculation will be added in separate PR)
- added a `SimulatedShowerDistribution` container, for storing the sim_telarray thrown shower historgrams.

* Update SimTelEventSource

- fill index containers correctly
- fix bug where missing input_url didn't always fil correctly.
- allow `~` and env vars in input filenames

* add "Set" to standard traits

* fix forgotten rename in test

* fix test using leakage parameters (renamed)

* fix tutorial

* fix a few style warnings

* change percent to fraction in LeakageContainer

* also fix test

* updated names of leakage parameters again

leakage_fraction_X -> leakage_pixels_X

* update leakage names in calculation

* again updated leakage names

* fixed one more place I forgot to rename

* used @maxnoe's suggestion of a Deprecated wrapper class

- added Deprecated
- changed deprecated members to use it
- also added  a "version" attribute
- added test

* help -> reason (to avoid keyword)

* removed accidental commit

* Revert "used @maxnoe's suggestion of a Deprecated wrapper class"

This reverts commit 541699b.

* add reason for deprecation and issue warning

* fix test

ctapipe/core/__init__.py

@@ -4,7 +4,7 @@
 """
 
 from .component import Component, non_abstract_children
-from .container import Container, Field, Map
+from .container import Container, Field, DeprecatedField, Map
 from .provenance import Provenance, get_module_version
 from .tool import Tool, ToolConfigurationError
 
@@ -13,6 +13,7 @@
     'Container',
     'Tool',
     'Field',
+    'DeprecatedField',
     'Map',
     'Provenance',
     'ToolConfigurationError',

ctapipe/core/container.py

@@ -2,6 +2,7 @@
 from copy import deepcopy
 from pprint import pformat
 from textwrap import wrap
+import warnings
 
 
 class Field:
@@ -28,12 +29,22 @@ def __init__(self, default, description="", unit=None, ucd=None):
         self.ucd = ucd
 
     def __repr__(self):
-        desc = f'{self.description}'
+        desc = f"{self.description}"
         if self.unit is not None:
-            desc += f' [{self.unit}]'
+            desc += f" [{self.unit}]"
         return desc
 
 
+class DeprecatedField(Field):
+    """ used to mark which fields may be removed in next version """
+    def __init__(self, default, description="", unit=None, ucd=None, reason=""):
+        super().__init__(default=default, description=description, unit=unit, ucd=ucd)
+        warnings.warn(f"Field {self} is deprecated. {reason}", DeprecationWarning)
+        self.reason = reason
+
+
+
+
 class ContainerMeta(type):
     """
     The MetaClass for the Containers
@@ -47,27 +58,24 @@ class ContainerMeta(type):
     """
 
     def __new__(cls, name, bases, dct):
-        field_names = [
-            k for k, v in dct.items()
-            if isinstance(v, Field)
-        ]
-        dct['__slots__'] = tuple(field_names + ['meta', 'prefix'])
-        dct['fields'] = {}
+        field_names = [k for k, v in dct.items() if isinstance(v, Field)]
+        dct["__slots__"] = tuple(field_names + ["meta", "prefix"])
+        dct["fields"] = {}
 
         # inherit fields from baseclasses
         for b in bases:
             if issubclass(b, Container):
                 for k, v in b.fields.items():
-                    dct['fields'][k] = v
+                    dct["fields"][k] = v
 
         for k in field_names:
-            dct['fields'][k] = dct.pop(k)
+            dct["fields"][k] = dct.pop(k)
 
         new_cls = type.__new__(cls, name, bases, dct)
 
         # if prefix was not set as a class variable, build a default one
-        if 'container_prefix' not in dct:
-            new_cls.container_prefix = name.lower().replace('container', '')
+        if "container_prefix" not in dct:
+            new_cls.container_prefix = name.lower().replace("container", "")
 
         return new_cls
 
@@ -142,10 +150,10 @@ def __setitem__(self, key, value):
 
     def items(self, add_prefix=False):
         """Generator over (key, value) pairs for the items"""
-        if not add_prefix or self.prefix == '':
+        if not add_prefix or self.prefix == "":
             return ((k, getattr(self, k)) for k in self.fields.keys())
 
-        return ((self.prefix + '_' + k, getattr(self, k)) for k in self.fields.keys())
+        return ((self.prefix + "_" + k, getattr(self, k)) for k in self.fields.keys())
 
     def keys(self):
         """Get the keys of the container"""
@@ -176,13 +184,14 @@ def as_dict(self, recursive=False, flatten=False, add_prefix=False):
             for key, val in self.items(add_prefix=add_prefix):
                 if isinstance(val, Container) or isinstance(val, Map):
                     if flatten:
-                        d.update({
-                            f"{key}_{k}": v
-                            for k, v in val.as_dict(
-                                recursive,
-                                add_prefix=add_prefix
-                            ).items()
-                        })
+                        d.update(
+                            {
+                                f"{key}_{k}": v
+                                for k, v in val.as_dict(
+                                    recursive, add_prefix=add_prefix
+                                ).items()
+                            }
+                        )
                     else:
                         d[key] = val.as_dict(
                             recursive=recursive, flatten=flatten, add_prefix=add_prefix
@@ -220,7 +229,7 @@ def __repr__(self):
             if isinstance(getattr(self, name), Map):
                 extra = "[*]"
             desc = "{:>30s}: {}".format(name + extra, repr(item))
-            lines = wrap(desc, 80, subsequent_indent=' ' * 32)
+            lines = wrap(desc, 80, subsequent_indent=" " * 32)
             text.extend(lines)
         return "\n".join(text)
 
@@ -239,17 +248,17 @@ def as_dict(self, recursive=False, flatten=False, add_prefix=False):
             for key, val in self.items():
                 if isinstance(val, Container) or isinstance(val, Map):
                     if flatten:
-                        d.update({
-                            f"{key}_{k}": v
-                            for k, v in val.as_dict(
-                                recursive, add_prefix=add_prefix
-                            ).items()
-                        })
+                        d.update(
+                            {
+                                f"{key}_{k}": v
+                                for k, v in val.as_dict(
+                                    recursive, add_prefix=add_prefix
+                                ).items()
+                            }
+                        )
                     else:
                         d[key] = val.as_dict(
-                            recursive=recursive,
-                            flatten=flatten,
-                            add_prefix=add_prefix,
+                            recursive=recursive, flatten=flatten, add_prefix=add_prefix
                         )
                     continue
                 d[key] = val

ctapipe/core/tests/test_container.py

@@ -1,52 +1,51 @@
 # Licensed under a 3-clause BSD style license - see LICENSE.rst
 import pytest
-from ctapipe.core import Container, Field, Map
+from ctapipe.core import Container, Field, Map, DeprecatedField
 
 
 def test_prefix():
     class AwesomeContainer(Container):
         pass
 
     # make sure the default prefix is class name without container
-    assert AwesomeContainer.container_prefix == 'awesome'
-    assert AwesomeContainer().prefix == 'awesome'
+    assert AwesomeContainer.container_prefix == "awesome"
+    assert AwesomeContainer().prefix == "awesome"
 
     # make sure we can set the class level prefix at definition time
     class ReallyAwesomeContainer(Container):
-        container_prefix = 'test'
+        container_prefix = "test"
 
-    assert ReallyAwesomeContainer.container_prefix == 'test'
+    assert ReallyAwesomeContainer.container_prefix == "test"
     r = ReallyAwesomeContainer()
-    assert r.prefix == 'test'
+    assert r.prefix == "test"
 
-    ReallyAwesomeContainer.container_prefix = 'test2'
+    ReallyAwesomeContainer.container_prefix = "test2"
     # new instance should have the old prefix, old instance
     # the one it was created with
-    assert ReallyAwesomeContainer().prefix == 'test2'
-    assert r.prefix == 'test'
+    assert ReallyAwesomeContainer().prefix == "test2"
+    assert r.prefix == "test"
 
     # Make sure we can set the class level prefix at runtime
-    ReallyAwesomeContainer.container_prefix = 'foo'
-    assert ReallyAwesomeContainer().prefix == 'foo'
+    ReallyAwesomeContainer.container_prefix = "foo"
+    assert ReallyAwesomeContainer().prefix == "foo"
 
     # make sure we can assign instance level prefixes
     c1 = ReallyAwesomeContainer()
     c2 = ReallyAwesomeContainer()
-    c2.prefix = 'c2'
+    c2.prefix = "c2"
 
-    assert c1.prefix == 'foo'
-    assert c2.prefix == 'c2'
+    assert c1.prefix == "foo"
+    assert c2.prefix == "c2"
 
 
 def test_inheritance():
-
     class ExampleContainer(Container):
         a = Field(None)
 
     class SubclassContainer(ExampleContainer):
         b = Field(None)
 
-    assert 'a' in SubclassContainer.fields
+    assert "a" in SubclassContainer.fields
 
     c = SubclassContainer()
     assert c.a is None
@@ -60,7 +59,6 @@ class SubclassContainer(ExampleContainer):
 
 
 def test_multiple_inheritance():
-
     class ContainerA(Container):
         a = Field(None)
 
@@ -70,12 +68,11 @@ class ContainerB(ContainerA):
     class ContainerC(ContainerB):
         c = Field(None)
 
-    assert 'a' in ContainerC.fields
-    assert 'b' in ContainerC.fields
+    assert "a" in ContainerC.fields
+    assert "b" in ContainerC.fields
 
 
 def test_override_inheritance():
-
     class ContainerA(Container):
         a = Field(1)
 
@@ -90,7 +87,6 @@ class ContainerB(ContainerA):
 
 
 def test_container():
-
     class ExampleContainer(Container):
         x = Field(-1, "x value")
         y = Field(-1, "y value")
@@ -117,8 +113,8 @@ class ExampleContainer(Container):
     assert cont.x == -1
 
     # test adding metadata
-    cont.meta['stuff'] = 'things'
-    assert 'stuff' in cont.meta and cont.meta['stuff'] == 'things'
+    cont.meta["stuff"] = "things"
+    assert "stuff" in cont.meta and cont.meta["stuff"] == "things"
 
 
 def test_child_containers():
@@ -134,7 +130,6 @@ class ParentContainer(Container):
 
 
 def test_map_containers():
-
     class ChildContainer(Container):
         z = Field(1, "sub-item")
 
@@ -154,7 +149,6 @@ class ParentContainer(Container):
 
 
 def test_container_as_dict():
-
     class ChildContainer(Container):
         z = Field(1, "sub-item")
 
@@ -167,18 +161,25 @@ class ParentContainer(Container):
     the_flat_dict = cont.as_dict(recursive=True, flatten=True)
     the_dict = cont.as_dict(recursive=True, flatten=False)
 
-    assert 'child_z' in the_flat_dict
-    assert 'child' in the_dict and 'z' in the_dict['child']
+    assert "child_z" in the_flat_dict
+    assert "child" in the_dict and "z" in the_dict["child"]
 
 
 def test_container_brackets():
-
-    class TestContainer(Container):
+    class ExampleContainer(Container):
         answer = Field(-1, "The answer to all questions")
 
-    t = TestContainer()
+    t = ExampleContainer()
 
-    t['answer'] = 42
+    t["answer"] = 42
 
     with pytest.raises(AttributeError):
-        t['foo'] = 5
+        t["foo"] = 5
+
+def test_deprecated_field():
+    class ExampleContainer(Container):
+        answer = DeprecatedField(-1, "The answer to all questions", reason="because")
+
+    cont = ExampleContainer()
+    cont.answer = 6
+

ctapipe/core/traits.py

@@ -4,7 +4,6 @@
 from traitlets import (
     Bool,
     CaselessStrEnum,
-    CRegExp,
     Dict,
     Enum,
     Float,
@@ -16,6 +15,8 @@
     TraitType,
     Unicode,
     observe,
+    Set,
+    CRegExp,
 )
 from traitlets.config import boolean_flag as flag
 
@@ -31,6 +32,7 @@
     "Long",
     "List",
     "Bool",
+    "Set",
     "CRegExp",
     "Dict",
     "flag",
@@ -52,20 +54,21 @@
 
 
 class Path(TraitType):
-    def __init__(self, exists=None, directory_ok=True, file_ok=True):
-        """
-        A path Trait for input/output files.
+    """
+    A path Trait for input/output files.
 
-        Parameters
-        ----------
-        exists: boolean or None
-            If True, path must exist, if False path must not exist
+    Parameters
+    ----------
+    exists: boolean or None
+        If True, path must exist, if False path must not exist
 
-        directory_ok: boolean
-            If False, path must not be a directory
-        file_ok: boolean
-            If False, path must not be a file
-        """
+    directory_ok: boolean
+        If False, path must not be a directory
+    file_ok: boolean
+        If False, path must not be a file
+    """
+
+    def __init__(self, exists=None, directory_ok=True, file_ok=True):
         super().__init__()
         self.exists = exists
         self.directory_ok = directory_ok