Fix bug in conditional advection for TripolarGrids

[simone-silvestri]

The new topologies were treated inconsistently with regards to inactive nodes and conditional advection.
This PR fixes it.

[codecov]

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.19%. Comparing base (9b5766c) to head (50b24fc).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...vection/topologically_conditional_interpolation.jl	25.00%	3 Missing ⚠️
src/Grids/inactive_node.jl	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5564      +/-   ##
==========================================
+ Coverage   65.58%   73.19%   +7.60%     
==========================================
  Files         403      403              
  Lines       22612    23932    +1320     
==========================================
+ Hits        14831    17516    +2685     
+ Misses       7781     6416    -1365     

Flag	Coverage Δ
buildkite	68.71% <33.33%> (+3.12%)	⬆️
julia	68.71% <33.33%> (+3.12%)	⬆️
reactant_1	6.41% <0.00%> (?)
reactant_2	11.34% <0.00%> (?)
reactant_3	9.74% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[briochemc]

I must admit I find these confusing. One thing I thought is could we use better names for the alias where the intent is clear? AFAIU, RC, which sounds like it stands for "RightConnected and friends" really is used here to collect topologies that have a given center/face sizes (grid.N for both) as opposed to LeftConnected which as an extra face (grid.N + 1). Could we apply the same logic as for FaceExtendedTopology and use a name that describes what characteristic of the grid/topology we are dispatching on?

I haven't checked the actual checks/indices/offsets yet. I'll try to do that at some point but I have other pressing things to do first so don't wait for me on that particular part! (Also if the language and the logic are simplified that will help me review that!)

[briochemc]

What about the distributed cohort? Should they be added here?

[simone-silvestri]

The distributed topologies are connected on each side (both left and right) so they always use the full stencil for advection computation (they behave as a Periodic topology)

[briochemc]

I see there were already a few like this existing before but I don't understand why not collapse the things like

(i >= $required_halo_size(adv)) & (i >= $required_halo_size(adv) - 1)

to just

(i >= $required_halo_size(adv))

?

[simone-silvestri]

good catch, I will do some cleanup here

[simone-silvestri]

I must admit I find these confusing. One thing I thought is could we use better names for the alias where the intent is clear? AFAIU, RC, which sounds like it stands for "RightConnected and friends" really is used here to collect topologies that have a given center/face sizes (grid.N for both) as opposed to LeftConnected which as an extra face (grid.N + 1)

No, in this case it is really "Connected on the Right". This is because we only need to check the left side and not the right, it does not have to do with the number of points it has, also the comment points it out.

[briochemc]

No, in this case it is really "Connected on the Right". This is because we only need to check the left side and not the right, it does not have to do with the number of points it has, also the comment points it out.

Oh OK, got it (I think). But then, why not use LeftBounded or something similar instead of RC if the characteristic that we dispatch on is boundedness? (I might still be confused so sorry if that's the case)

[simone-silvestri]

Oh OK, got it (I think). But then, why not use LeftBounded

What about LB? For left bounded