Generalize type hints in signal/dataset classes to explicitly support custom collections

[funbotan]

I propose replacing List type hints with Sequence for signal class constructor parameters. I noticed that the way these parameters are used is consistent with Sequence.

There are some use cases where passing a collection that is not a List is beneficial. For example, when using DynamicGraphTemporalSignal datasets, the changes in the graph structure may occur only once in hundreds or thousands of timesteps, and copying the same data so many times is extremely memory-inefficient. Here is a proof-of-concept class that a user may want to use in such a situation:

class DeduplicatedList(MutableSequence):
    """
    This is a somewhat confusing and poorly implemented collection that I
    do not recommend copying. Its purpose is to efficiently store a list
    that consists of long contiguous ranges of repeating elements.
    """

    def __init__(self):
        self.data = OrderedDict()
        self.index = 0

    def __getitem__(self, index: int):
        while index < 0:
            index += len(self)
        for rng, item in self.data.items():
            if index in rng:
                return item
        raise IndexError

    def __setitem__(self, index: int, value: _T) -> None:
        raise NotImplementedError

    def __delitem__(self, index: int) -> None:
        raise NotImplementedError

    def __iter__(self):
        for rng, item in self.data.items():
            for _ in rng:
                yield item

    def __len__(self) -> int:
        return self.index

    def insert(self, index: int, value: _T) -> None:
        """
        Pardon the confusion, but this method actually _appends_ `index`
        elements `value` to the collection. This one just has a more
        suitable signature than `append`.
        """
        self.data[range(self.index, self.index + index)] = value
        self.index += index

Of course, the proposed change does not change the functionality in any way; it only removes a potential source of confusion for the users and explicitly shows that they may use a Sequence of their choice. Therefore, no tests should be required.

[codecov-commenter]

Codecov Report

Merging #212 (c8ba7df) into master (a581323) will not change coverage. The diff coverage is 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master     #212   +/-   ##
=======================================
  Coverage   99.31%   99.31%           
=======================================
  Files          57       57           
  Lines        4980     4980           
=======================================
  Hits         4946     4946           
  Misses         34       34           

Impacted Files	Coverage Δ
...ric_temporal/signal/dynamic_graph_static_signal.py	97.43% <100.00%> (ø)
...mporal/signal/dynamic_graph_static_signal_batch.py	95.23% <100.00%> (ø)
...c_temporal/signal/dynamic_graph_temporal_signal.py	100.00% <100.00%> (ø)
...oral/signal/dynamic_graph_temporal_signal_batch.py	95.29% <100.00%> (ø)
...poral/signal/dynamic_hetero_graph_static_signal.py	98.90% <100.00%> (ø)
...signal/dynamic_hetero_graph_static_signal_batch.py	100.00% <100.00%> (ø)
...ral/signal/dynamic_hetero_graph_temporal_signal.py	100.00% <100.00%> (ø)
...gnal/dynamic_hetero_graph_temporal_signal_batch.py	100.00% <100.00%> (ø)
...ic_temporal/signal/static_graph_temporal_signal.py	100.00% <100.00%> (ø)
...poral/signal/static_graph_temporal_signal_batch.py	95.12% <100.00%> (ø)
... and 2 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[benedekrozemberczki]

Thank you!