Refactor to allow halo > 0 decomposition of the mesh #20
Conversation
author twsearle <14909402+twsearle@users.noreply.github.com> 1706276681 +0000 committer twsearle <14909402+twsearle@users.noreply.github.com> 1716902914 +0100 gpgsig -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAABAgAGBQJmVdwDAAoJED1uBW/R3NJPlUsP/0IIAUjVAMDkhK+6QH7U6cH3 p7tjftrwTTkc3NnrtkSM/NvnbtOSKFxKobojBE0wpTyi6L6zX2e51yDwUuVG27D5 vqoql/ywe5DBhMPCjwnyhOOqhsu7tRdnzGz/FwSqLXm5t1RE/D+XCZo1b+Beb7S8 G4V1jVMzO/i8y+0h+VWux64Ls7BbMo2Tkcaf18+fgp0nuhO7ECEFV1bAAwskVgO3 z+wX3udYWXSEIYP26AQ37wxCTE6gsOnBDPcO5Mc6s4Ic6zfRwUDmy8Q7CgyP2YP2 vWUN8Fz9kmXKmSaxDUmu0qtbAlV7olbZ9Z+jkL/WKVSmGGzqTvs+y44dYj1Qouql d9HHVZ1qrn0K9GGv7fSRqUBxA0DnWKd3vV5WX0OfAEkSRo/BJ17lWOJjONDNkEXY vzdVePWigLT399k4f1GtAHVZ1UJ09tQGiRBhEFQKz0D/wqcJy46Tk1pJ5sgG/lmI BSyO7f4lKLEsSWXuthirzcJ0kzTMxcVX5nj3asGP4XUw1tKsaWIu2LtVszpaEZaN 8CI2UvswnpV/iN7IvXAFmSlxWNrupQYNxaRnnX15s3QDOmKsNeHfZvAViFGd9Bgx sOrfYPHPlbo4wccSjtcM+RBHH4j043G0HRyQ+fh3hlwvvRDxHzV3cB2ZUdqqg+84 YwLvb38qdltNEdPw5+ay =DxeJ -----END PGP SIGNATURE----- * factor out surrounding rectangle class * separate indexing of the surrounding rectangle from the indexing of orca grid * better handling of grid wrapping for calculating rectangle extent * split out dimensions of grids * split out new classes to manage local grid info * Local ORCA grid setup * preliminary tests of the orca grid * print diagnostics to files * Delineate between owned and used nodes in the local orca grid * Use .at array access to catch out-of-bounds errors * Reset setting of ORCA halo to ghost points to the expected logic. It is unknown why the existing logic was as it is, and this should be clarified if possible. * add halo to surrounding rectangle * halfway through adding halos to LocalOrcaGrid * run localOrcaGrid test for halo = 1 * tighten adjustments of localOrcaGrid when halo=0
* Update to surrounding rectangle to handle halos. * Ensure assert statements are only applied in the correct situation. * handle periodic points in the halo that are on my partition. * Ensure coordinates are wrapped. --------- Co-authored-by: Toby Searle <14909402+twsearle@users.noreply.github.com>
* Updated logic. * Add comment. * Add testing of halo = 1 to test_orca_mesh_boundaries.cc. * Update for mesh boundary test with halos. * Deal with edge case at bottom of grid. * Fix test logic. * Rename function to make its purpose clearer. * Update function name in test.
|
|
* Add test with halo = 2 and fix two bugs. * Remove if statement that has a small chance of producing unexpected side effects. * Expand output to help diagnose issues.
There was a problem hiding this comment.
I think splitting these parts up is a very good idea @twsearle thanks for taking this up!
I applied a few fixes to your branch related to normalisation and IJ-indexing into the north-fold halo.
I have done my best to understand the changes - apologies if I am misunderstanding this stuff is quite confusing and I made the changes over quite a large block of time - some of the functions/choices I made might be redundant. I will rerun this version to double check it gives us the same results in our integration testing. During the process we thought we were done with this change and then found a whole extra series of issues with higher resolution grids. Its a real slog to get through this review thanks so much for your time! Let me know if there is anything else I can do. |
|
It is definitely not an easy piece of code to understand and edit. |
I think everything is OK - I just checked and we are still passing our integration tests. Sorry for being super cautious about this one (I can see on reflection your changes only affect the fields used to view in gmsh) - I have thought I had it about a dozen times, made a minor change, and broken something somewhere! It has been rough. I think the dodgy looking cells are fundamentally the fault of the degeneracy in the orca grid as I mentioned above. I have regrets about choosing the ORCA2 grid for testing - its a very strange animal. |
|
@wdeconinck We are now using atlas v 0.38.1 🎉 in our stack! This means that we finally have everything in place to use this change once it is merged and tagged. Apologies again for my wordy comments/descriptions on this one, it really was rather painful! However we have been working with it for a while now and I feel much more confident with it now. Is there anyone else who needs to review it? |
There was a problem hiding this comment.
Thanks @twsearle for confirming all that! Good to merge then.
|
Thanks very much for merging this @wdeconinck could we please get a new tag version including this change? I am excited to get it into our stack as soon as possible! |
Description
Split the mesh partitioner into components:
SurroundingRectangle(better name might be LocalLonLatGrid, green box) that surrounds the points held by this partition.LocalOrcaGridobject that includes any orca periodic points at the edges of the domain owned by the partition.This allows us to set the halo at the generation of the surrounding rectangle, and separate out the logic to handle the strangeness of the problems at the orca grid edge.
Note, there were some strange rogue points in the lower portion of the grid that were not seemingly correctly marked as ghost points. This has had no impact to correct, other than it affects our calculation of field norms in orca-jedi.
Issues
Acceptance criteria
Testing