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

Reshape method #1760

Merged
merged 44 commits into from
Mar 11, 2025
Merged

Reshape method #1760

merged 44 commits into from
Mar 11, 2025

Conversation

artv3
Copy link
Member

@artv3 artv3 commented Nov 5, 2024

Summary

This PR adds helper methods to views to create Reshape methods with compile time options C and Fortran style indexing

@artv3 artv3 changed the title reshape method Reshape method Nov 5, 2024
@artv3
Copy link
Member Author

artv3 commented Dec 23, 2024

@MrBurmark , @rhornung67 I think the example for the reshape method can be simplified. I just pushed up a new version. I was a bit worried the previous version might have too much going on.

@artv3
Copy link
Member Author

artv3 commented Mar 6, 2025

Just pushed up an iteration which ties Reshape to the permuted layout usage in view.

@rchen20
Copy link
Member

rchen20 commented Mar 6, 2025

I like this idea: it makes permutations and layouts much easier to use. However, I think it should be called something like ViewMaker, or something else. When I see Reshape, I think that this will take an existing View I've already defined, and will alter its dimensions. Instead, this PR is creating a wrapper that returns a new View, and can be used to construct Views from scratch. I realize that ReShape is used in a similar way in MFEM as well, so if everyone else is on board with this then that's fine with me too.

@rhornung67
Copy link
Member

I like this idea: it makes permutations and layouts much easier to use. However, I think it should be called something like ViewMaker, or something else. When I see Reshape, I think that this will take an existing View I've already defined, and will alter its dimensions. Instead, this PR is creating a wrapper that returns a new View, and can be used to construct Views from scratch. I realize that ReShape is used in a similar way in MFEM as well, so if everyone else is on board with this then that's fine with me too.

That's actually a good point. What about make_permuted_view? We have several make_*_view helpers and this would be consistent with what we already have.

@artv3
Copy link
Member Author

artv3 commented Mar 6, 2025

I like this idea: it makes permutations and layouts much easier to use. However, I think it should be called something like ViewMaker, or something else. When I see Reshape, I think that this will take an existing View I've already defined, and will alter its dimensions. Instead, this PR is creating a wrapper that returns a new View, and can be used to construct Views from scratch. I realize that ReShape is used in a similar way in MFEM as well, so if everyone else is on board with this then that's fine with me too.

That's actually a good point. What about make_permuted_view? We have several make_*_view helpers and this would be consistent with what we already have.

Maybe we can debate this at the RAJA Hackathon, this falls in line with documentation and our desire to perform general clean up.

@rhornung67
Copy link
Member

Coming up with good, intuitive names is hard.

@MrBurmark
Copy link
Member

MrBurmark commented Mar 6, 2025

I second make_permuted_view. That's more in line with our existing API. I assume reshape as a name comes from the function in eigen?

@rchen20
Copy link
Member

rchen20 commented Mar 6, 2025

Another idea would be to add this functionality as partial specializations of the existing make_view function, and get rid of the ReShape object entirely. What's implemented here is a way to create both permuted and non-permuted Views, so the functionality is generic enough to sit under the make_view umbrella.

@artv3
Copy link
Member Author

artv3 commented Mar 10, 2025

Thank you all, through teamwork we have improved the API and implementation details. I believe we are ready for another review.

@artv3
Copy link
Member Author

artv3 commented Mar 10, 2025

@MrBurmark, if you have a second can you provide a review here? I think we are good to go once you take a look.

@artv3 artv3 enabled auto-merge March 11, 2025 00:17
@artv3 artv3 merged commit 9834d7d into develop Mar 11, 2025
27 checks passed
@artv3 artv3 deleted the artv3/reshape branch March 11, 2025 11:30
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.

5 participants