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

Introduce aliasing for mdspan #3116

Conversation

schnellerhase
Copy link
Contributor

This encapsulates the access to the mdspan functionality. This increases readability of code, as types are clearer and reduces the necessary changes to exchange the underlying mdspan implementation, for example after it becomes part of the STL.

@schnellerhase schnellerhase force-pushed the refactor_mdspan_for_type_alias branch 2 times, most recently from cea2909 to 980c06e Compare March 28, 2024 17:27
@francesco-ballarin
Copy link
Member

francesco-ballarin commented Mar 29, 2024

Other people will comment in more detail, but to me:

  1. it feels odd to have those alias in the dolfinx namespace, considering that they are in dolfinx/common and most of what's there is in the dolfinx::common namespace
  2. it feels odd to have an alias here, but not in basix, which is where the mdspan header is.

@jhale
Copy link
Member

jhale commented Mar 29, 2024

I think these symbols should go in dolfinx::common::mdspan then we can do a quick search and replace to std:: when that is possible.

@jhale
Copy link
Member

jhale commented Mar 29, 2024

Further to @francesco-ballarin second point it might be better to have these aliases once in Basix.

@schnellerhase
Copy link
Contributor Author

Moved all of the aliasing to namespace dolfin::common as suggested.

I understand that the same aliasing may be done in basix, but that does not imply that one should refrain from aliasing again in dolfinx. So at some point in the dolfinx code we should be specific about which mdspan implementation is to be used and enforce it via the alias, independent of it aliasing MDSPAN_IMPL_STANDARD_NAMESPACE::mdspan or a possible future basix::mdspan.

Shall I open an issue for the suggested mdpsan aliasing in basix?

@jhale
Copy link
Member

jhale commented Mar 30, 2024

I think it would be better to have a namespace dolfinx::common::mdspan rather than sticking all of these in ::common. It would also make switching to std:: very easy.

@jhale
Copy link
Member

jhale commented Mar 30, 2024

We will use std::mdspan as soon as it is widely available.

@schnellerhase
Copy link
Contributor Author

Ah missed that you meant that as the full namespace identifier - will change.

@garth-wells
Copy link
Member

I don't think this is a good idea. We wouldn't change std::vector into dolfinx::vector. Happy with something that shortens long namespaces, but we shouldn't appropriate a 3rd-party library.

@schnellerhase schnellerhase force-pushed the refactor_mdspan_for_type_alias branch 2 times, most recently from 41fe7af to c69c0ae Compare March 30, 2024 12:08
@schnellerhase
Copy link
Contributor Author

schnellerhase commented Mar 30, 2024

No definitely one wouldn't change the vector aliasing, but only since its completely standardized by the required C++ version of the project.

This does not apply for mdspan in the currently used, C++20 standard. For now mdspan is a 3rd party library (made available through basix) that mimics a future standardized implementation and might be changed for an improved or backwards compatible version. So I'd argue that this rectifies an alias for the clean separation of use cases of mdspan insside dolinfx and the 3rd party backend implementation.

This new namespace includes the aliases associated to the mdspan type, naminlgy
- mdspan
- dextents
- extents
- full_extent
- dynamic_extent
- submdspan
@jhale
Copy link
Member

jhale commented Mar 31, 2024

I just took a look at what Kokkos is doing and their strategy for managing this (and eventually coexistence with other implementations) and I don't think we should try and be too clever, see e.g.:

kokkos/mdspan#297
kokkos/mdspan#253

@schnellerhase
Copy link
Contributor Author

So, I guess the 3rd party style interaction for the mdspan implementation got voted down, even though I'd still argue that it is the way to go. But that also implies that there is no nice way of encapsulating the namespace macros away better than by the macro usage that already is present, so I close this MR?

@jhale
Copy link
Member

jhale commented Apr 10, 2024

I'm happy to revisit this when we get to the point of switching to the included std::mdspan across our target platforms. It was a useful discussion so thanks @schnellerhase for bringing it up.

@jhale jhale closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants