Fixed #2708 - Smoke tests failing on 1D and 2D grids #2994
Conversation
There was a problem hiding this comment.
@clyne - All tests are passing. I'm curious to better understand what the fix was. I see the Grid classes are simplified through removing _nDims and some new logic for terrain following curvilinear grids. Could you summarize what the problem was? Thanks.
| long newIndexL = Wasp::LinearizeCoords(_index.data(), _dims.data(), _dims.size()) + offset; | ||
| if (newIndexL < 0) { newIndexL = 0; } | ||
| if (newIndexL > maxIndexL) { | ||
| _index = {0, 0, 0}; | ||
| _index[_nDims - 1] = _dims[_nDims - 1]; | ||
| _index = {0, 0, _dims[_dims.size() - 1]}; |
There was a problem hiding this comment.
Is _dims[_dims.size() - 1] still needed since all of our grids are now 3D in nature? IE - Would _dims[2] not work?
There was a problem hiding this comment.
It's just good programming practice not to use hard coded values. I can't imagine why we'd need to change the size of DimsType in the future, but if we do we won't have to search for every place where "2" occurs :-)
There were still a few instances in the code that assumed that the size of 'dims' or 'index' determined the grids topology. This is no longer the case. However, now that DimsType is always an array of length 3, handling of anomalous cases where one of the dimensions is of unit length is somewhat problematic. For a dimension NxMx1 you can simply ignore the 3rd dimension. But what should be done with Nx1xM or 1xNx1? You can't simply ignore the unit length because the position now matters. This is what was tripping up gridTests.cpp; it was assuming that if the Grid was 2D the unit dimension must be the 3rd dimension. The Grid class and Grid iterators will allow a non-unit dimension that is not the last (slowest varying) dimension. I thought about prohibiting this (and may yet still), but what if you want to describe a "layered" grid that has only one horizontal dimension (i.e. the vertical axis and only one of the X or Y axes are non-unit length)? This is possible with the current Grid code, but only if unit dimensions are permitted for X or Y. So the Grid classes now handle all the outlier test cases that the testGrid generates. But most of rest of the code base does NOT. There are many places in the that assume if a Grid is topologically 2D, then the first two positional indices are how you address it. This won't work if the unit dimension is in the first or second position. If a use case arises where we need to support a terrain following grid with a unit-length horizontal dimension we will need think harder about this. For now, all of the configurations in testGrid are supported and the Grid class is a little more flexible than it was. |
|
@clyne thanks for the detailed explanation. |
No description provided.