kokkos / array_ref

Polymorphic multidimensional array view
36 stars 9 forks source link

Question about the implementation #22

Closed mbianco closed 8 years ago

mbianco commented 8 years ago

Thanks Carter for the updated version,

In the implementation there is a decoupling between layout types and offsets. This would make extendability a little complex since a user should define a tag-type plus an implementation as partial specialization of a (std::) library template. And you still need to model some offset concept. What I would prefer is to have the layout-offset to model the offset concept so that the extendability does not have to pass through a specialization. The user just have to implement a template class with a give interface. What's the rationale behind the current implementation?

I think this would make even easier to produce a generic affine layout in which the order of the strides are specified at compile time, so that layout_left and layout_right could be simple alias templates of this generic class (I should provide an implementation, shouldn't I?). We are using something similar in our library, a stripped example of implementation can be found in https://github.com/crosetto/examples_cpp/blob/master/Code/Solution/storage_info_sol.hpp#L30 in which the layout map specifies the permutation to apply to the indices in order to get the offsets.

Does any of this make sense to anybody?

hcedwar commented 8 years ago

I absolutely agree with the need to make layout extension / specialization as straightforward as possible. The prototype could definitely be better in this regard. Would be helpful if you could put an implementation in the source repository. The paper itself currently leaves it as a planned TBD.

mbianco commented 8 years ago

I started an initial implementation in branch https://github.com/kokkos/PolyView/tree/layout-offsets I had to use a different interface, since layout is required to be a complete type. Will think more about it. The major change is the dimension is given as argument to the layout. The rest of the test is the same.

mbianco commented 8 years ago

I provided another implementation that does not change the file test_view.hpp in branch https://github.com/kokkos/PolyView/tree/forwarding-layouts

In this version the layout type is not a simple tag, but instead provide the offset class as an alias with the name offset. A user wanting to customize the layout, then need to provide a layout class and a offset class. The offset alias must accept a single template argument that models a Dimension concept.

In the other branch the user had to provide a single class modeling a LayoutOffset concept.

Anyway, I need more work there.

mbianco commented 8 years ago

Ok, take a look at it. Change in API: the order of layout and dimension is not prescribed. If this is not desired I'm pretty sure it can be fixed. I did this way so it was easier to code. https://github.com/kokkos/PolyView/tree/forwarding-layouts Let me know what you think.

hcedwar commented 8 years ago

I definitely prefer your extensibility strategy. Ordering of properties is flexible; however, there should be style guidelines which could be enforced - just takes enforcement code.

hcedwar commented 8 years ago

Should we elaborate the layout extensibility strategy in the current paper, or follow up with a subsequent paper? Tradeoff is to not overwhelm with detail vs. insuring that implementations work from the same extensibility strategy from the beginning.

mbianco commented 8 years ago

My problem was that leaving a detail like extensibility for future versions would have two effects: first the committee could be suspicious about the design, second, even though they accept the design, maybe this design requires some API changes that would need to pass through the committee a second time, which would upset people, I think. Having few words about extensibility in this paper could help, I think

hcedwar commented 8 years ago

This layout extension strategy has been incorporated into the proposal.