Skip to content

Conversation

@yasahi-hpc
Copy link
Collaborator

This PR fixes the base int type of map in transpose helper.
Previously, it only worked for std::array<int, DIM>.
Now, it works also for std::array<std::size_t, DIM>

@yasahi-hpc yasahi-hpc self-assigned this Nov 7, 2025
@yasahi-hpc yasahi-hpc added bug Something isn't working cleanup labels Nov 7, 2025
@PaulGannay
Copy link

Since the map is holding index value used to access views, why do you need a new type in addition to IndexType?

@yasahi-hpc
Copy link
Collaborator Author

Since the map is holding index value used to access views, why do you need a new type in addition to IndexType?

Map does not hold the index value used to access views.
It holds the permutation array that maps src and dst index arrays.

m_out(src_idx[m_map[Is]]...) = m_in(src_idx[Is]...);

As you said, src_idx must be in IndexType, but int type of the m_map can be different.

@PaulGannay
Copy link

Why offer a customisation point for this?
Why not use std::size_t internally?

@yasahi-hpc
Copy link
Collaborator Author

Why offer a customisation point for this? Why not use std::size_t internally?

Actually, I am considering that. Although APIs allow std::array<int, DIM> to specify axes, they should be converted into std::array<std::size_t, DIM> internally
Since it requires more changes, I would like to do that in another PR

@PaulGannay
Copy link

Actually, I am considering that. Although APIs allow std::array<int, DIM> to specify axes, they should be converted into std::array<std::size_t, DIM> internally
Since it requires more changes, I would like to do that in another PR

I would close this PR without merge then.
But if you think that it is necessary, in preparation of other work, I haven't seen anything wrong either so I guess that you can merge it.

@yasahi-hpc
Copy link
Collaborator Author

Actually, I am considering that. Although APIs allow std::array<int, DIM> to specify axes, they should be converted into std::array<std::size_t, DIM> internally
Since it requires more changes, I would like to do that in another PR

I would close this PR without merge then. But if you think that it is necessary, in preparation of other work, I haven't seen anything wrong either so I guess that you can merge it.

It is useful for distributed implementation details, wherein all the axes are already converted into std::array<std::size_t, DIM>. If you prefer, I can change this function to work on std::array<std::size_t, DIM> and convert maps into std::array<std::size_t, DIM>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants