API cleanup to use 2D arrays for Set/GetDOFs#339
Conversation
…ordinateView::GetCoordinates -> GetValues Squashed original commits: 7986df9, c063a3a
Rather than having 2D views always default to layout_right, we have views default to use the default layout of whatever memory_space they are created over. I.e., layout_left for GPU and layout_right for CPU. This makes them interoperable with the Kokkos created views.
| size_t len = num_rows * static_cast<size_t>(num_components); | ||
| Rank1View<const T, DeviceMemorySpace> subspan{ | ||
| flat.data_handle() + offset, len}; | ||
| data.data_handle() + row_offset * static_cast<size_t>(num_components), |
There was a problem hiding this comment.
Although the tests pass, I'd like to double-check that this flattening is correct or not, since DeviceMemorySpace may use LayoutLeft now. For example, with 3 rows and 2 components, a LayoutLeft view would be like x00 x10 x20 x01 x11 x21 in memory. When row_offset == 1, data.data_handle() + row_offset * num_components would point to x20 rather than x10. Am I missing something here?
There was a problem hiding this comment.
Good catch. I thought I caught everywhere that was using data_handle() we need to check on what order the mesh fields expects. If, the layout mesh fields expects is not the same as what's contiguous in our data, then we will need to copy the appropriate data into a contiguous array.
Can you make sure we have a test that is exercising this code? We might be getting away with it because we are loading, then unloading the data using the same ordering. However, we we try to access in an integration routine or directly access in mesh fields the data will be scrambled.
There was a problem hiding this comment.
Oh...Probably we don't hit this because we mostly only test things with scalar (1 component) which is effectively a Rank1 array. Try writing a multicomponent test case.
There was a problem hiding this comment.
Would it cause any issues if I add a commit directly to this PR branch? I'm not sure if it might make rebasing more difficult later, since other PRs are based on it.
There was a problem hiding this comment.
It should not create any issues if you checkout and push to this branch (jacobmerson/set-get-2d-arrays). Only thing that could potentially cause issues is if to many people rewrite history on the same branch at the same time (and I'm not planning on updating this branch, unless you need me to do something).
There was a problem hiding this comment.
Supporting multiple components and addressing the layout mismatch turned out to require more changes than I initially expected. Since the changes in this PR make sense for the current single-component implementation, I'll address the multi-component support and layout issues in a separate PR (See issue #342 ). This PR is good to merge as is.
Sichao25
left a comment
Sichao25
left a comment
There was a problem hiding this comment.
Overall looks good. I have one concern about the layout assumptions used when flattening the device Rank2View.
2 participants
No description provided.