Add aggregated read functionality

[znicholls]

Pull request

Please confirm that this pull request has done the following:

• Tests added
• Discussed tests added with co-authors
• Documentation added (where applicable)
• Example added (either to an existing notebook or as a new notebook, where applicable) (N/A as low level)
• Description in CHANGELOG.rst added
• make black

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <http://link-to-pr.com>`_) Added feature which does something
- (`#XX <http://link-to-pr.com>`_) Fixed bug identified in (`#XX <http://link-to-issue.com>`_)

[znicholls]

@swillner close #92 and #86 before looking at this (builds on both those PRs)

[znicholls]

@swillner are you happy with the tests that have been added?

[swillner]

@swillner are you happy with the tests that have been added?

Very! ;)

[znicholls]

@swillner this is good to go from my end

[swillner]

I am afraid there are some issues with this approach:

• Why do we need to catch NameError?
• The unit converter converts between this parameter's unit and the requested one, so the one of the child parameter might be different (same for the timeframe)
• Once we attempt an aggregated view, shall we allow added children?
• Minor: Return type is missing
• Minor: We should probably make this a protected ("_"-prefixed) method
• Major: Rather than summing and then caching the sum in self._parameter._data I would collect views to the children's values and sum them on get (comes with the penality of always doing the sum for each get, even if there have been no changes, but the conversion is done and changes in the child parameters reflect on a new get as is the idea of the parameters views)
• The recursion can be much simplyfied similar to:

    def _sum_child_data(self) -> float:
        """
        Sum child data.

        Returns
        -------
        float
            Sum of all child data
        """
        if self._parameter._children:
            data = 0
            for _, cp in self._parameter._children.items():
                data += self._unit_converter.convert_from(cp._sum_child_data()) # TODO fix unit conversion
        else:
            return self._parameter._data

(similar for timeseries).

[znicholls]

* Why do we need to catch NameError?

We don't, it's just another way to do these loops where you initialise in the first iteration then add later. These are all equivalent I think

from timeit import default_timer as timer

def sum_iterable(nums):
    data = 0  # you have to know the type in advance
    for j, a in enumerate(nums):
        data += a

    return data

start = timer()
numbers = [1, 3, 5]
for i in range(10000):
    res = sum_iterable(numbers)

print("result = {}".format(res))
end = timer()
print("{:.2f}ms".format((end - start) * 1000))

~7ms

from timeit import default_timer as timer

def sum_iterable(nums):
    for j, a in enumerate(nums):
        data = a if j == 0 else data + a

    return data

start = timer()
numbers = [1, 3, 5]
for i in range(10000):
    res = sum_iterable(numbers)

print("result = {}".format(res))
end = timer()
print("{:.2f}ms".format((end - start) * 1000))

~8ms

from timeit import default_timer as timer

def sum_iterable(nums):
    for a in nums:
        try:
            data += a
        except NameError:
            data = a

    return data

start = timer()
numbers = [1, 3, 5]
for i in range(10000):
    res = sum_iterable(numbers)

print("result = {}".format(res))
end = timer()
print("{:.2f}ms".format((end - start) * 1000))

~10.23ms

Looking at above I'll move to initialising as numpy arrays can add to scalars without issue so initialising with zero shouldn't be a problem.

[znicholls]

* The unit converter converts between this parameter's unit and the _requested one_, so the one of the child parameter might be different (same for the timeframe)

good pick up, will change

* Once we attempt an aggregated view, shall we allow added children?

Thinking about it more, not sure. Yes means more flexibility but you can get confused as the same view will give different answers depending on when it's called.

* Minor: Return type is missing

Yep

* Minor: We should probably make this a protected ("_"-prefixed) method

Yep

* Major: Rather than summing and then caching the sum in `self._parameter._data` I would collect views to the children's values and sum them on `get` (comes with the penality of always doing the sum for each get, even if there have been no changes, but the conversion is done and changes in the child parameters reflect on a new `get` as is the idea of the parameters views)

nice

[znicholls]

Alright let's have another go. I think the only thing I didn't implement from your suggestions is this.

Once we attempt an aggregated view, shall we allow added children?

Thinking about it more, not sure. Yes means more flexibility but you can get confused as the same view will give different answers depending on when it's called. Maybe that's the behaviour we want, how much do we want to 'lock the low level'/do views normally imply the data doesn't change?

[swillner]

still having a lot of comments. mind if i make a pr to this one later?

[znicholls]

still having a lot of comments. mind if i make a pr to this one later?

yep, no rush

[swillner]

Thinking about it more, not sure. Yes means more flexibility but you can get confused as the same view will give different answers depending on when it's called. Maybe that's the behaviour we want, how much do we want to 'lock the low level'/do views normally imply the data doesn't change?

Well, the point of the views is that they yield the most up-to-date data for each read via get so the values should be able to change especially when new data is written to them (e.g. for a new run). But adding a new child is something different....

[swillner]

Just saw, that I disallow adding child parameters once a parameter has been read from anyway, which makes sense for non-aggregating reads, so we just apply the same rationale here.

[znicholls]

@swillner should be good to go now

[swillner]

nice!