Skip to content

[2/13][Adjoint Module] Round the grid origin to a whole pixel - #3298

Open
smartalecH wants to merge 1 commit into
fix/adjoint-adjacent-material-gridsfrom
fix/source-boundary-deposition
Open

smartalecH wants to merge 1 commit into
fix/adjoint-adjacent-material-gridsfrom
fix/source-boundary-deposition

Conversation

@smartalecH

@smartalecH smartalecH commented Sep 2, 2026 •

Copy link
Copy Markdown
Collaborator

Supersedes #3290.

Fixes #1051.

Root cause. grid_volume::set_origin(const vec&) rounds to the nearest half pixel, so any geometry_center that is not a whole number of pixels gives the grid an odd origin io (e.g. 0.1 at resolution 15 is 1.5 px). The Yee lattice assumes io is even — icenter() documents this and center_origin() enforces it — and loop_in_chunks (vec2diel_floor) and grid_volume::interpolate place each component on absolute ivec parities. With an odd io those disagree with where fields and materials are actually stored, so:

  • sources are deposited half a pixel from where they are evaluated (materials are not affected),
  • get_field_point, DFT monitors and other loop_in_chunks readouts are likewise half a pixel off,
  • source deposition depends on the chunk layout, because the chunk-owned ranges no longer tile the source's interpolation lattice.

Fix. Round the origin to a whole pixel, as center_origin() already does, and round Simulation.geometry_center the same way in Python so the two agree. The rounding is documented.

An earlier version of this PR instead patched loop_in_chunks to realign each chunk with the absolute lattice (the parity shift discussed in review). That made the odd-io case chunk-independent but left sources and monitors half a pixel off, so it has been replaced by this root-cause fix (prototyped in #3333).

Evidence (2D/3D, all six components; point, line, plane and boundary-straddling sources on/off chunk splits; random non-pixel geometry_center):

master master, geometry_center pre-rounded this PR
split vs. unsplit, 80 random configs 23/80 differ 0/80 0/80
-np 3/-np 4 vs -np 1, 40 configs — — bit-identical
#1051 script vs. translated reference 0.9916 vs 0.753 — 0.741437 vs 0.741436

The middle column shows that master's source-deposition code is already chunk-independent once io is even.

Behavior change. A non-pixel geometry_center now snaps to the nearest pixel instead of the nearest half pixel — the same order of accuracy, but now consistent.

Tests. Chunk-boundary tests in test_source.py (zero-thickness and point sources on a chunk boundary with a half-pixel geometry_center, compared against a single chunk), plus test_geometry_center_rounded_to_pixel in test_simulation.py.

@smartalecH smartalecH changed the title [2/n][Adjoint Module] Fix source deposition across chunk boundaries [2/13][Adjoint Module] Fix source deposition across chunk boundaries Sep 2, 2026
Comment thread src/sources.cpp Outdated
Comment thread src/loop_in_chunks.cpp Outdated
ivec _iscoS(S.transform(gvu.little_owned_corner(cS), sn));
ivec _iecoS(S.transform(gvu.big_owned_corner(cS), sn));
ivec _iscoS(S.transform(use_symmetry ? gvu.little_owned_corner(cS)
: gvu.little_corner() + gvu.iyee_shift(cS),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: gvu.little_corner() + gvu.iyee_shift(cS) is the first point in the chunk for that component, whether it is owned or not.

It would be good to add a comment explaining why this is needed for the !use_symmetry case.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added -- let me know if this comment works.

Comment thread src/loop_in_chunks.cpp Outdated
if (parity) iscS.set_direction(d, iscS.in_direction(d) + 1);
if ((iecS.in_direction(d) - (is - shifti).in_direction(d)) % 2)
iecS.set_direction(d, iecS.in_direction(d) - 1);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little bit weird? Won't shifting by 1 shift it to a different Yee grid?

@smartalecH

smartalecH commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

@stevengj your question about the parity shift in loop_in_chunks led me rethink some things. So Claude and I explored other ways to solve this problem.

geometry_center sets the grid origin with set_origin(round_vec(...)), which rounds to the nearest half pixel. So any non-pixel geometry_center gives an odd io, breaking the even-origin assumption that icenter() documents. That is why the +1 here was needed: it realigns each chunk with the absolute lattice used by vec2diel_floor. But that lattice is half a pixel from the stored fields, so even with this PR, sources and loop_in_chunks/interpolate readouts (DFT monitors, get_field_point) are half a pixel off when io is odd. This PR makes that case chunk-independent, not correct.

I've opened #3333 as a draft alternative. It rounds the origin to a whole pixel, as center_origin() already does, and replaces all of this PR's C++ changes with a ~5-line change in grid_volume::set_origin, keeping the same chunk-boundary tests. On master with geometry_center pre-rounded, the existing source-deposition code was already chunk-independent in all 80 random configurations I tried, so the chunk-boundary machinery here is unnecessary once io is even. Details and numbers are in #3333.

The trade-off is that a non-pixel geometry_center now snaps to the nearest whole pixel rather than half pixel. Would you prefer that, or keeping half-pixel origins and making the lattice code io-aware instead (a much larger change)? If #3333 is the way to go, I'll replace this PR with it.

I'm also not sure this is in line with the "illusion of continuity," but I may be overthinking things here...

Thanks!

@stevengj

stevengj commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

The trade-off is that a non-pixel geometry_center now snaps to the nearest whole pixel rather than half pixel.

That seems fine. Either way it is the same order of accuracy.

@smartalecH
smartalecH force-pushed the fix/source-boundary-deposition branch from 08f14c5 to 500815f Compare October 9, 2026 20:20
@smartalecH smartalecH changed the title [2/13][Adjoint Module] Fix source deposition across chunk boundaries [2/13][Adjoint Module] Round the grid origin to a whole pixel Oct 9, 2026
geometry_center was rounded to the nearest half pixel, so a non-integer
number of pixels gave the grid an odd origin. The Yee lattice assumes an
even origin (see grid_volume::icenter), so sources and monitors were
placed half a pixel from the fields and materials, and source deposition
depended on the chunk layout.

Fixes #1051.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@smartalecH
smartalecH force-pushed the fix/source-boundary-deposition branch from 500815f to 650d0b9 Compare October 9, 2026 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simulation result depends on # of processes, using geometry_center

2 participants