-
Notifications
You must be signed in to change notification settings - Fork 168
Update UxGrid search API to match BaseGrid #2066
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
WRT testing ravel/unravel, we have a few tests that check evaluation of a field from our generic data sets; under the hood, this exercises both raveling and unraveling. Are you thinking of tests that just exercise ravel/unravel ? |
Yes. In the xgrid case I have a test that checks that roundtrips ravel and unravel and checks that it generates unique ei's . Maybe that's overkill, I can also see that POV |
|
That makes sense though. The eval call stack involves a lot of other things and having these kinds of highly directed tests can help us further characterize what could be going wrong in eval (for example). |
Considering now I don't think round tripping ravelling is actually needed right now for uxgrid - with #2056 ravel and unravel could then be tested on the BaseGrid level (and also make testing easier for xgrid and uxgrid). Any other review comments/thoughts for this PR? |
fluidnumerics-joe
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.
Quite a tidy change. All good on my end
Continuing the work in #2037 to fix #2048, this PR defines the axes for UxGrid as
["Z", "FACE"]. See #2048 for the full contextChanges:
ei, bcoordsand replace witthposition)Open questions:
In terms of naming, should we go with "FACE" or "CELL"? Is there convention here?
Do you feel that there should be more test cases for ravelling/unravelling on an unstructured grid @fluidnumerics-joe ? (kinda what is done for structured grids at the moment?). If yes, I don't think its a high priority - though stubs would be helpful so we don't forget
Chose the correct base branch (
v4-devfor v4 changes)Fixes A clear API for handling grid axes, barycentric coordinates, index searching, and ravelling. #2048
Added tests - None (yet)