Conversation
sethrj
left a comment
There was a problem hiding this comment.
I think your pull request makes a really good point, since more of our classes are indeed transforming directly from a C++ vector into a "range" of grids, and there's a fair bit of duplication and checking. But I fear that this implementation is overly strict (it basically requires all the data to behave that way, if I'm not mistaken) and it obfuscates the construction of the data itself.
I can't think of a way to simplify the construction meaningfully unless we're doing the exact same data layout and construction in multiple classes... which doesn't yet seem to be the case.
I'm happy to check in over zoom and chat about possibilities...
| CELER_ENSURE(data_); | ||
| } | ||
|
|
||
| CollectionMirror<P> data_; |
There was a problem hiding this comment.
After many years of working with complex code bases, I've learned to hate protected data because it almost universally breaks encapsulation: it makes it hard to determine where the data is coming from, who owns it, and who's allowed to modify it. So I intentionally wrote ParamDataInterface as a pure abstract interface rather than a concrete class. Using pure abstract base classes (ABCs) means we don't have to worry nearly as much about multiple inheritance (which is very nice for a lot of use cases: e.g., a class that can have "begin run", "execute", "aux data", and "output").
Even though it saves a few lines in the class, it also makes it harder to see that the class itself has host ref, device ref functions: using ABCs forces all the functions to be defined by the daughter class, which substantially improves code readability IMHO.
| */ | ||
| CerenkovParams::CerenkovParams(SPConstProperties properties) | ||
| template<> | ||
| class DataBuilder<CerenkovData> : public GridDataBuilder<CerenkovData> |
There was a problem hiding this comment.
I don't think this works correctly unless you declare the specialization in the header file... maybe. The interplay between the DataBuilder and the fancy Params is very tricky...
|
|
||
| return import_angle_integral(host_ref.reals[ri_grid.value], | ||
| host_ref.reals[ri_grid.grid]); | ||
| }); |
There was a problem hiding this comment.
I'm not sure I see the advantage of this form: it makes it less easy to follow (not obvious that it's a loop, not obvious that the argument to the lambda is the one item from the second argument to the ListBuilder).
What about something like a std::for_each or std::transform combined with std::back_inserter?
This is a rough draft of a different way to do generic grid building. The main objectives are:
Params --> make host value --> build data on host --> mirror dataalmost everywhere, so we can remove some repeated code with a templated builder.I found that most of the logic is looping over a list of IDs (usually materials or particles) and then building a specific data struct from a corresponding list of imported data. I wanted the
ListBuilderto take in a range of valid IDs to loop over, so that once (say) the number of optical materials has been determined, we can check every time we build a collection indexed by optical materials that the correct number of materials is provided as imports.