Remove neighbour duplication#87
Conversation
Previously, neighbours across periodic boundaries were tracked in separate data structures (_neighbours_p, _sendPos_p, _recvPos_p, _cornerNeighbours_p, _cornerSendPos_p) with a dedicated getNeighbourInfoPeriodic() method and separate NetCDF output paths. This change merges periodic neighbour info into the same maps used for non-periodic neighbours, removing redundant code and data structures: - Removed getNeighbourInfoPeriodic() entirely - Removed periodic dimension/variable definitions from saveMetadata() - Removed periodic data writes from saveMetadata() - Consolidated periodic and regular neighbours into _neighbours, _sendPos, _recvPos, _cornerNeighbours, _cornerSendPos
TomMelt
force-pushed
the
remove-neighbour-duplication
branch
from
d7ac15e to
8a28401
Compare
…bdas - Replace duplicated edge/corner loop logic with templated compute_dims() - Introduce DimInfo struct to group neighbour dimension data - Add lambda helpers (def_dim, write_array, write_scalar) to reduce repetitive nc_* calls
TomMelt
force-pushed
the
remove-neighbour-duplication
branch
from
8a28401 to
b154a94
Compare
Mikolaj-A-Kowalski
left a comment
Mikolaj-A-Kowalski
left a comment
There was a problem hiding this comment.
Just couple of comments that came to mind after a first look.
There was a problem hiding this comment.
I am not sure if my impression is correct. But the _px and _py suffixed partition tests differ from their base version only by being periodic?
I it is the case I am thinking we should probably remove them.
Also maybe open a followup issue to provide some description of the tests (i.e. what they intend to test). Unless I missed something, at the moment, it is missing.
| info.dims.resize(items.size(), 0); | ||
| info.offsets.resize(items.size(), 0); | ||
| for (std::size_t i = 0; i < items.size(); i++) { | ||
| info.numNeighbours[i] = (int)data[items[i]].size(); |
There was a problem hiding this comment.
| info.numNeighbours[i] = (int)data[items[i]].size(); | |
| info.numNeighbours[i] = static_cast<int>(data[items[i]].size()); |
since C++ style cast offer additional type safety and are much easier to 'grep' for?
| auto def_dim = [&](const std::string& name, int len, int& dimid) { | ||
| NC_CHECK(nc_def_dim(nc_id, name.c_str(), len, &dimid)); | ||
| }; |
There was a problem hiding this comment.
| auto def_dim = [&](const std::string& name, int len, int& dimid) { | |
| NC_CHECK(nc_def_dim(nc_id, name.c_str(), len, &dimid)); | |
| }; | |
| auto def_dim = [&](const std::string& name, int len) { | |
| int dimid; | |
| NC_CHECK(nc_def_dim(nc_id, name.c_str(), len, &dimid)); | |
| return dimid; | |
| }; |
It may be personal bias, but mutable references as arguments always feel to me a bit off 😅
The logic being is that a call site it is never obvious in C++ that a particular I will quote what made argument can be modified by a function. Since we are dealing with a pure intent(out) situation here, returning feels more explicit in terms of the intention.
| NC_CHECK(nc_close(nc_id)); | ||
| } | ||
|
|
||
| struct DimInfo { |
There was a problem hiding this comment.
I am thinking this helper class could use a short doc-comment to describe its intent a bit.
To a novice reader (like myself) it is not completely obvious what each field represents.
I will stare at it a bit more and try to phrase.
|
|
||
| // Define periodic dimensions in netCDF file | ||
| std::vector<int> dimids_p(N_EDGE); | ||
| NC_CHECK(nc_def_dim(nc_id, "P", _totalNumProcs, &dimid)); |
There was a problem hiding this comment.
Can we use the def_dim lambda here as well?
| // Define groups in netCDF file | ||
| int bbox_gid, connectivity_gid; | ||
| // ---- Define groups ---- | ||
| int bbox_gid, c_grid; |
There was a problem hiding this comment.
I have to say I liked the old name better :-(
I am not sure why did we change it?
Mikolaj-A-Kowalski
left a comment
Mikolaj-A-Kowalski
left a comment
There was a problem hiding this comment.
Ok. I kind of have a feeling that we can spend eternity trying to improve this function ;-), but ultimately it does not matter much.
So I would say it is ready to go just with the few suggestions for consideration ;-)
What I am thinking we REALLY need to do is to have some proper description of the output format (i.e. what are the groups/variables in the net cdf file and what do they mean). I think it is outside the scope of this PR, but if you agree we should open an issue and try to address it soon.
2 participants
Unify periodic and non-periodic neighbour info
Closes #84
Summary
Merged periodic neighbour tracking into the same data structures used for non-periodic neighbours. Since a neighbour cannot be both periodic and non-periodic at the same time, separate
_psuffixed maps were redundant.Changes
getNeighbourInfoPeriodic()and merged all periodic neighbour data (halo sizes, send/recv buffer positions, corner neighbours) into the existing_neighbours,_sendPos,_recvPos,_cornerNeighbours, and_cornerSendPosmaps._neighbours_p,_sendPos_p,_recvPos_p,_cornerNeighbours_p,_cornerSendPos_pmember variables and the periodic overload ofgetNeighbourInfoPeriodic().saveMetadata— replace duplicated edge/corner loops with a templatedcompute_dims()function; introduceDimInfostruct to group neighbour dimension data; use lambda helpers (def_dim,write_array,write_scalar) to reduce repetitivenc_*callsTests
Updated all reference CDL files to match the unified neighbour output (periodic neighbour IDs now appear in the standard (non-periodic) fields)
TODO