New cubed sphere 2 grid #245
Conversation
|
|
There was a problem hiding this comment.
Looks good to see a much simpler CS grid! I've added some changes. It's mostly style related. I'm sure @wdeconinck can make some more comments/suggestions.
After this, we should probably plumb the grid into the grid builder.
@wdeconinck, what do you think?
|
@wdeconinck, would you be able to have a look at this at your earliest convenience? |
I'm haunted by the realisation that "earliest convenience" sounds super passive aggressive. What I meant was, "whenever you can spare the time". 🙃 |
There was a problem hiding this comment.
Sorry it has taken me this long @mo-jonasganderton , @odlomax . It looks great! I just have a few minor nitpick-issues to be addressed.
Down the line I hope to remove the number "2" everywhere as in CubedSphere2 :)
|
Thank you @wdeconinck! I've made a series of improvements following your comments, let me know if there's anything else. |
| Vector tan_point(3); | ||
| Vector xyz(3); | ||
|
|
||
| tan_point << tan_coord[0], tan_coord[1], 1; | ||
|
|
||
| xyz = lfric_rotations_transposed_[tile] * tan_point; | ||
| xyz.normalize(); | ||
|
|
||
| return {xyz(0), xyz(1), xyz(2)}; |
There was a problem hiding this comment.
I guess this can be unrolled avoiding any temporary Vector and also removes the need for a maths library.
PointXYZ xyz;
const auto& mat = lfric_rotations_transposed_[tile];
// xyz = mat * Vector{tan_coord[0], tan_coord[1], 1}
xyz[0] = mat(0,0) * tan_coord[0] + mat(0,1) * tan_coord[1] + mat(0,2);
xyz[1] = mat(1,0) * tan_coord[0] + mat(1,1) * tan_coord[1] + mat(1,2);
xyz[2] = mat(2,0) * tan_coord[0] + mat(2,1) * tan_coord[1] + mat(2,2);
xyz.normalize();
return xyz;There was a problem hiding this comment.
Apologies to return to this again. It is just much better to avoid any object that may be causing allocations. I think that's the case when Eigen is not used. Also the Matrix type could now just be in fact a std::array<std::array<double,3>,3> and we don't need to include any eckit/maths or Eigen in CubedSphere2.h
There was a problem hiding this comment.
I'm on board with this. It's probably my fault for falling into the trap of thinking ahead about things we might not actually need. An array of arrays is more portable and easy enough to invert if needed.
There was a problem hiding this comment.
It is also always possible to create a non-owning Eigen Matrix view of std::array<std::array<double,3>,3> if you so desire :)
There was a problem hiding this comment.
All done!
wdeconinck
force-pushed
the
feature/cubed_sphere_2_grid
branch
from
f92c927 to
a2d9c76
Compare
wdeconinck
force-pushed
the
feature/cubed_sphere_2_grid
branch
from
a2d9c76 to
2efb785
Compare
* release/0.41.0: (44 commits) Update changelog Version 0.41.0 Suppress more Intel warnings Suppress more Intel warnings Fix more warnings with gnu 13.2 Release build Fix warnings (#256) Move SparseMatrixStorage template functions in .cc Add atlas-global-matrix sandbox Add AssembleGlobalMatrix Add SparseMatrixToTriplets Fix SparseMatrixStorage constructor from SparseMatrixView Fix creation of spectral function space from TransLocal (fixes #249) (#254) Fortran: Allow to create MatchingPartitioner with derived FunctionSpaces Remove spurios debugging output Mesh constructor with Distribution New simplified cubed sphere grid (#245) Fix for StructuredPartitionPolygon inner bounding box Add hicSparse backend to sparse matrix multiply. (#246) Update atlas-run runner: remove old aprun command and update slurm (#251) Avoid no_discard warning ...
Description
Initial implementation of a new cubed sphere grid.
@odlomax and @wdeconinck can both have a look over to recommend any changes - I expect this will have changed once everything else has also been implemented.
Using this implementation generates the following lonlat points - n=12, red=tile0, purple=tile5, darkest point of each colour is tij=0, lightest point is tij=143. This matches the sequence of the points in the LFRic cubed sphere.
Impact
Once everything else is also implemented, this makes the cubed sphere more maintainable.
Checklist