-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix full modal nonlinear polarisation #247
Conversation
Ok this definitely does not work. I get the wrong answer when pumping in LP11 but with HE11 present. With Kerr only, there should be no nonlinear polarisation in HE11, but I get the same as in HE21 and TM01: This is because HE11 and HE21/TM01 don't have the same symmetry, so we do actually need to integrate (0, 2π) for the integral to vanish. I think our best option for now is actually to switch to julia> @btime transform_vert(nl_vert, Eω_vert, 0.0);
266.604 ms (8471 allocations: 99.19 MiB)
julia> transform_vert.ncalls
527 Without this PR, but switching to julia> @btime transform_vert(nl_vert, Eω_vert, 0.0);
155.324 ms (5195 allocations: 60.84 MiB)
julia> transform_vert.ncalls
323 I think it's possible that when @jtravs measured the performance of |
I'm much happier with this, and I think you're correct about my benchmarks. I will review once I see the tests have passed. |
Wait, why are tests skipped? |
Tests were skipped because I marked this as draft. They're running now. |
There is still a stack overflow error on the v1.5 tests, but I'm pretty certain it's not this PR (see #248 ) so I'll merge. I think the error is due to one of our dependencies getting a minor update. Need to check in more detail. |
This should (I think) fix #246
dimlimits
forMarcatiliMode
s are now (0, π) rather than (0, 2π). There is a new functionModes.geomfac
which takes this into account by multiplying the integral inTransModal
by 2. For other modes this defaults to 1. With this change, the initial points at whichpcubature
evaluates the integrand are now (0, π/2, π), so it no longer gets stuck.I still have to fully convince myself that this is general--so far it produces correct results for both LP11 propagation (new test in
test_polarisation_field.jl
and HE11 propagation (test_multimode.jl
).As it stands this will mess up derived modes, like for example a
ZeisbergerMode
, because they don't implementgeomfac
. So the brute-force method of calculatingModes.N
andModes.Aeff
no longer produces correct results.TODO:
geomfac
into the loop inTransModal
to avoid the extra array multiplicationdimlimits
the same