Repository navigation
Simple support for tripolar grid topology - #96
Mikolaj-A-Kowalski wants to merge 20 commits into
Conversation
|
The tests passed locally I promise (failing on CI like that is nothing short of a public betrayal! I have a feeling we will be on bad terms going forward...) Will try to have a look for the cause of the discrepancy later. EDIT: |
TomMelt
left a comment
TomMelt
left a comment
There was a problem hiding this comment.
Thanks @Mikolaj-A-Kowalski on initial inspection it looks good to me. You have found the most minimal way to implement the feature and improved some of the existing functionality
Having an arithmetic operators of the Points will simplify any geometrical operations. Point reflection is the geometric operation required by the tripolar topology (it is what happens at the top edge). Extra construction function is required to build 'valid' domains from any two points. We don't want to impose it though since it is likely we will expand the Domain to represent "empty" intersections. In that case we would like to allow points p1 and p2 not satisfy invariant: `p1.x <= p2.x && p1.y <= p2.y` which we will be able to use to represent empty domain.
Together with the utility methods to check the dimensionality(?) of the domain ( i.e. if it represents an area, line, point or is empty) we can now easily compute the neighborhood relations between the domains.
Only use SUBCASE where there is a need to a common set-up/tear-down.
A boolean switch that indicates that grid follows a tripolar topology. It is exclusive with the periodicity settings. It implies: - Periodic left and right boundary - Non-periodic bottom boundary - Special 'folded' periodic top boundary
A simple case without any land and 2 ranks. This results in a simple split in the middle of the domain. Note that when domains touch by a single corner point, like in the case of this test, there are two possible positions in the send buffer. Hence this test may fail even when correct data is produced if the convention which point should be chosen is altered.
This implementation follows the 'path of least resistance' and tries to modify the minimal amount of existing code required to get things working. We can determine the neighbours by comparing against their image in the 'reflected' regon above the TOP edge. In tripolar topology, to produce the reflection we apply a point symmetry through the middle of the top edge. We only need to take care to use the right 'real domain'/'image' combination when calculateing the send/recv buffers.
We do it with a global attribute in the metadata file. It is optional.
It is failing due to bugs in corner Send buffer location calculations.
Implements a simple scheme based on checking the halfspaces containing the extreme edges of the domain. The implementation is still quite ugly and should be refactored.
218d51b to
251cddd
Compare
|
Rebased on the current main to get rid of the commits coming from other PRs this one was based on. |
Since function was working on a point, it was a bit ambiguous what does the point refer to in the old name.
It is not used at the moment so there is no reason to have it linger in the codebase. This commit may need to be reversed if a need for this function ever arises.
|
@jatkinson1000 Tagging for review. I think it is ready for a look. |
jatkinson1000
left a comment
jatkinson1000
left a comment
There was a problem hiding this comment.
Hey @Mikolaj-A-Kowalski there is nothing insane to my reading here, so happy for this to be merged, with the regular caveat that I am not deeply familiar with decomp yet.
I'd suggest opening issues for potential follow-up work on perfection, but let these sit for future development as we need to get this delivered.
Disclosure, I did not read the cdl changes closely so trust you here.
This is just a note for the future. No need to make it an issue since it will become relevant only if there is a 2nd implementation of the Partiotioner interface.
|
Ok @jatkinson1000 @niravshah241 I think this one is ready to go in. If we are happy I will squash the fixup comments follow by squash and merge. Note to self, I would need to prepare a branch with the |
|
Sounds like a plan! |
3 participants
This PR is stacked on #95 and #87
Resolves #86
Adds a 'least effort' support for the tripolar topology, here I have aimed to minimise any modification to the existing code.
I am also sneaking a
set -uin the integration test script since it improves error messages when one tries to run it locally without all the env variables set ;-)TODO:
xsize of the tripolar grid must be even. This (and potentially other) assumptions we are relying on need to be verified. I am thinking during theGridconstructionintersectionfunction. At the moment it is not used (and hence should not be commited yet). I haven't done it yet since getting it out of the history requires more work and I wanted to have this PR opened ASAP@TomMelt I think it would be a good to have a quick review of this PR, in particular 2nd pair of eyes to spot some edge cases that might appear would be appreciated. Especially if we can translate them into test.
I want to hold off polishing this PR to after I can create a asister draft PR in the nextsim which will implement the halo exchange with the tripolar data.