Make remove_vortices reach the farfield so the documented callback does something - #208
Make remove_vortices reach the farfield so the documented callback does something#208steps-re wants to merge 1 commit into
Conversation
holodyne#207 made the removal run, but the GS loop recomputes phase_ff from the untouched farfield in _gs_farfield_routines later in the same iteration, so the corrected phase is discarded before it can influence anything. Push the corrected phase back into the farfield, and add a test that runs the callback as documented and checks the optimization result changes.
|
correcting myself, and apologies for the noise. I ran a proper adversarial pass over this after opening it and found two things wrong, so the patch operates on a stale once the callback is actually live it hurts two of the four methods. over 15 seeds, the underlying report still holds: used as the docstring shows, the removal is discarded also, I got the credit wrong at the top. I wrote that #207 fixed "half" of #205. it fixed happy to rework this, or close it and leave you the report, whichever is less work for you. |
#207 fixed everything I reported in #205, including the unconditional
plot_farfieldinpoint 2, which its description does not claim. Working on top of it I hit a separate
problem it does not cover, and which was not in #205: used as a
callback=the way thedocstring shows, the removal still does not change the optimization.
the loop calls the callback right after
_nearfield2farfield, and then_gs_farfield_routinesdoeslater in the same iteration, unless
fixed_phaseis on. so whatever the callback wroteinto
phase_ffgets overwritten from the farfield, which nobody touched, before anythingreads it.
on main at 42a9b4b, GS 50 iterations, 256 px SLM, 512 px farfield, quadratic seed,
callback every 10 iterations:
identical to three decimals is what a no-op looks like. the removal itself works, it just
does not survive the iteration:
each call knocks the count down and the next one finds it back where it was or higher,
because the farfield the count is recomputed from was never touched.
the patch in this PR is not the right fix
I am leaving it up as a draft because the problem above is real and reproducible, but the
change itself has two faults I found only after opening the PR. flagging both rather than
letting anyone review it as-is.
it operates on a stale
phase_ff._nearfield2farfieldpopulatesself.farfieldbutdoes not refresh
phase_ff. only_gs_farfield_routinesdoes, at the end of the previousiteration. the callback runs in between, so at that moment
phase_ffdescribes theprevious farfield, measured max 3.14 rad and mean 0.24 rad out of sync with the current
one. writing it back therefore replaces this iteration's propagated phase with a
de-vortexed copy of the last one. that is not what my own description above says it does.
once the callback actually does something, it hurts two of the four methods. over 15
seeds, WGS-Leonardo goes from 189 vortices without the callback to 228 with it, and
WGS-Kim from 216 to 232. efficiency drops in every method and every seed, worst on MRAF at
about 14% relative. on stock the callback is a no-op for GS, Leonardo and Nogrette and a
net help for Kim, so this patch turns a harmless no-op into a regression on two of them.
nothing in the PR tests the WGS variants, which is why I did not see it.
so the direction needs to be: re-derive
phase_fffromself.farfieldbefore removing,and work out what the feature should do for the WGS variants at all, rather than enabling
it everywhere. happy to rework it, or to close this and leave you the bug report, whichever
you prefer.
for what it is worth the mechanical parts of the patch do check out: amplitude preserved to
about 1.5 float32 ulp, dtype preserved, no NaN introduced, exact zeros stay zero, stats
untouched (they read
amp_ff, cached before the callback), and thefixed_phasepath isbyte-identical to stock.
tests/holographyis 8 failed, 66 passed, 6 skipped, 11 errors on this branch against8 failed, 65 passed, 6 skipped, 11 errors on 42a9b4b. only the new test differs. the
failures and errors are environment, not code: 11 errors from a missing
pytest-benchmarkand 8 failures from a missing
imageio.repro:
benchmark_callback.py,run against stock main.
found with AI assistance, checked against the source and the numbers above. apologies for
the two corrections here, and for crediting #207 with only half of #205 in my first draft.