Conversation
This commit introduces two helper classes that reduce the verbosity of `std::enable_if<...>` constructs.
280d929 to
83bcfb1
Compare
matz-e
left a comment
There was a problem hiding this comment.
Looks good to me. load_unordered seems a fitting name, I guess something like load_optimized or just bulk_load would be obfuscating the biggest caveat (not loading the list as specified).
|
Back to draft, because the internals can be simplified. |
The point of this API is to enable collections to load morphologies
in any order they see fit. For HDF5 containers tests have shown that the
access pattern matters a lot in terms of performance.
The idea is a generic API that allows optimizing the iteration order for
reorderable loops, e.g.,
for k, morph_name in enumerate(morphology_names):
morph = collection.load(morph_name)
f(k, morph)
can be replaced with
for k, morph in collection.load_unordered(morphology_names):
assert collection.load(morphology_names[k]) == morph
f(k, morph)
This commit only adds the minimum required for this to work. In
particular, it doesn't optimize the access pattern.
This commit implement the optimized access order for merged containers.
ab7c787 to
1891e88
Compare
|
This PR now also includes an optimized version for merged containers. |
| template <class U = M> | ||
| typename enable_if_mutable<U, std::pair<size_t, M>>::type operator*() const; | ||
|
|
||
| void operator++() const; |
There was a problem hiding this comment.
prob. not important, but I tend to like the symmetry of having operator++(int)
There was a problem hiding this comment.
Okay, then we'd probably better also change the return type to const Iterator& and Iterator (or whatever the proper return types are), otherwise the distinction makes little sense.
There was a problem hiding this comment.
We've had to redo the LoadUnordered<M>::Iterator, it had multiple bugs related to unintended shallow copying. Tests have been added to check for this type of issue.
Also this restructures the file such that we declare in the header, then in the source file first we define; and then explicitly instantiate.
|
Should we allow random access? This would matter in loops which look like this: But technically the above doesn't traverse the morphologies in the optimal order. However, if we make the iterator shared, then we have more thread-safety concerns. What would be more useful in TD would be access to the internal |
|
Seams reasonable? Although, for "user-friendliness", maybe just sorting the morphology names without a round-robin trip through indices on the user side would be nicer? |
|
Sorting the morphology names directly loses valuable information, e.g. it prevents one from looking up auxiliary information if things are stored in vectors of equal size, with the convention that index More concretely in TD we have the case that we store stuff in a big |
527fc9d to
d2cd472
Compare
This commit allows users to perform an argsort of the morphology names they want to load. This can be useful in situations where it's hard to pass the iterator, e.g. through multiple layers of function calls.
d2cd472 to
83b91f3
Compare
78def01 to
89a76ea
Compare
This MR proposes an API which will enable us to optimize morphology loading for certain reorderable loops. The target are loops which know upfront the names of the morphologies they need to load, and don't require them to be loaded in a particular order.
The types of optimization this unlocks are: