-
Notifications
You must be signed in to change notification settings - Fork 366
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
Convert Box to BoxND #4016
Convert Box to BoxND #4016
Conversation
I added |
AMReX-Codes/pyamrex#345 merged - thanks! |
*static_cast<Long>(length(1)), | ||
*static_cast<Long>(length(2))) | ||
: Long(0); | ||
AMREX_IF_ON_HOST((checkOverflow();)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we put this inside ifdef AMREX_DEBUG
? I don't think we need to check for overflow here for non-debug builds, given it's very unlikely overflow could happen here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added defined(AMREX_USE_ASSERTION)
as for me debug mode is often too slow to use.
Src/Base/AMReX_Box.H
Outdated
amrex::min(b1.bigend [2], b2.bigend [2])), | ||
typ); | ||
#endif | ||
BoxND<dim> minBox (BoxND<dim> const& b1, BoxND<dim> const& b2, IndexTypeND<dim> typ) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this function. I don't think it's being used anywhere now. The name of this function is very misleading. So to avoid confusion, let's just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All minBox functions or just that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just this one. This one has different behavior from others. It was added by me 5 years ago. I made a mistake in naming.
LGTM. I will do one more pass tomorrow. |
Summary
Similar to #3969 and #3988 but for Box.
Additional background
It should be checked that the changes to BoxIndexer do not affect the compiled GPU code.
In my testing, it gives the same performance as development.
Example usage:
Checklist
The proposed changes: