-
Notifications
You must be signed in to change notification settings - Fork 62
Pull BasisIsCollocated helper to interface level #1868
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
Conversation
jrwrigh
left a comment
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.
Also need to update the release notes.
| *is_collocated = true; | ||
|
|
||
| for (CeedInt i = 0; i < basis->P_1d; i++) { | ||
| *is_collocated = *is_collocated && (fabs(basis->interp_1d[i + basis->P_1d * i] - 1.0) < 10 * CEED_EPSILON); |
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.
What if the 1D basis and quadrature points aren't assigned? Is interp_1d and the like always defined?
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.
Those are always assigned for a tensor basis. Impossible to make a tensor basis without them
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.
Sure. I'm thinking about this for non-tensor basis, which can have collocated a collocated basis-quadrature pair.
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.
currently not something we've implemented so we'd have to revisit when we find a use case
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.
My point is that a function called CeedBasisIsCollocated() should say whether the basis is collocated rather than whether it's collocated and tensor. I know this is a backend function, but it should still be clear what it does.
This would also make it more clear which cases are implemented and which are not in the backend code. Something like
CeedBasisIsTensor(basis, &is_tensor);
CeedBasisIsCollocated(basis, &is_collocated);
if (is_tensor && is_collocated) {
... // optimized code path (e.g. CeedBasisApply == memcpy)
} else {
... // normal code path
}is more clear than if we relied on CeedBasisIsCollocated() to be the entire determination of whether to take a more optimized code path.
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.
Sure, but we don't currently have any non-tensor collocated sort of logic, so I don't want to spend too much effort trying to get ahead of use cases that don't currently exist
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.
Fair. The limitation should be documented then.
Checking a non-tensor basis is not implemented and will always return false
7ec5c84 to
aa4b4a9
Compare
This will help add this to gen/shared cleanly