Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interoperability between static and dynamic rank views #295

Closed
etphipp opened this issue May 19, 2016 · 9 comments
Closed

Interoperability between static and dynamic rank views #295

etphipp opened this issue May 19, 2016 · 9 comments
Assignees
Labels
Feature Request Create new capability; will potentially require voting
Milestone

Comments

@etphipp
Copy link
Contributor

etphipp commented May 19, 2016

With the DynRankView and View now being used in a bunch of code together, interoperability between the two is becoming an issue. In particular some code only works with DynRankViews, other code only works with View's, and the user has a mixture of the two. Thus it would be nice if you could:

  • Assign a DynRankView to a dimension compatible View and vice versa
  • deep_copy() between the two
  • Compute the rank in an agnostic fashion: DynRankView has a rank() function but View has a rank enum. Often the only thing preventing code from working with both is this difference in how the rank is computed. Since View already has an enum called rank, you probably can't add rank() as a member function. So maybe a non-member function Kokkos::rank(v) with overloads for View's and DynRankView's?
@etphipp
Copy link
Contributor Author

etphipp commented May 19, 2016

@kyungjoo-kim

@hcedwar
Copy link
Contributor

hcedwar commented May 19, 2016

The DynRankView is supposed to be a temporary workaround (hack) until Intrepid2 can be refactored to remove the requirement for dynamic rank view.

@etphipp
Copy link
Contributor Author

etphipp commented May 19, 2016

That's part of the reason why you need interoperability. Only parts of the code are refactored, so you need a way to convert between the two.

Even so, the DynRankView will have general utility, so it would be a shame to get rid of it after Intrepid2 (and the codes that use it) have been refactored.

@ndellingwood ndellingwood self-assigned this May 19, 2016
@ndellingwood ndellingwood added the Feature Request Create new capability; will potentially require voting label May 19, 2016
@ndellingwood ndellingwood added this to the Summer 2016 milestone May 19, 2016
@kyungjoo-kim
Copy link
Contributor

Oh... I thought that the dynamic rank view is a permanent solution that everybody uses the same container among different packages, i.e., panzer, intrepid, drekar and albany. I don't think that we remove dynrankview after refactoring; introducing dynrankview is part of refactoring.

assignment between dynrankview and view may not be possible. but deep_copy should be okay if kokkos supports it.

@rppawlo
Copy link
Contributor

rppawlo commented May 20, 2016

I too was under the impression that DynRankView is a permanent thing. I certainly believe that the bracket operator should be removed once the intrepid conversion is done, but as Eric said, DynRankView is very useful.

@ndellingwood
Copy link
Contributor

When assigning a view to a dynamic rank view, make sure padding carries through.

@ndellingwood
Copy link
Contributor

Brief update: @hcedwar @crtrott @rppawlo @etphipp @kyungjoo-kim
Working on the some of the interoperability feature requests (subdynrankview to subview, copy and assignment of View to DynRankView) started to require multiple design workarounds, including some unacceptable within the Kokkos View (e.g. exposing the shared allocation tracker). After talking with Carter, the best route forward will be to redesign the DynRankView so that it no longer privately inherits the rank 7 View but will essentially be a new form of View (rank 7) that shares most all of its internal implementation with the original View.

@ndellingwood
Copy link
Contributor

@etphipp @rppawlo
The following features were added with #359

DynRankView redesigned to be its own View implementation
Added features:

  • Copy, Copy-assign View to DynRankView
  • deep_copy between Views and DynRankViews
  • subview function name now enabled (subdynrankview has same functionality)
  • Common method to request rank of a View and DynRankView: rank(user_view); This will eventually be replaced by a constexpr rank() in the View, but this will require coordination with packages and apps to avoid breaking their builds.

Important!
Before merging into Trilinos corresponding changes in Sacado must also be made as those in the attached patch file. Otherwise compatibility will break.

sacado-mod-drvredesign-7.patch

@ndellingwood
Copy link
Contributor

The patch didn't seem to go through, one more try...

sacado-mod-drvredesign-7.patch.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Create new capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

5 participants