Skip to content
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

Check for invalid sources at r=0 in cylindrical coordinates #2392

Merged
merged 6 commits into from
Feb 10, 2023

Conversation

oskooi
Copy link
Collaborator

@oskooi oskooi commented Feb 7, 2023

Adds a check within fields::add_volume_source src_vol_chunkloop for two three different types of invalid sources at $r=0$ in cylindrical coordinates.

@oskooi
Copy link
Collaborator Author

oskooi commented Feb 7, 2023

There is a single failing test in test_adjoint_cyl.py because of an $E_r$ line source extending in $r$ which includes $r=0$ for an $m=1.2$ simulation:

source_center = [design_r / 2, 0, -(sz / 2 - dpml + design_z / 2) / 2]
source_size = mp.Vector3(design_r, 0, 0)
src = mp.GaussianSource(frequency=fcen, fwidth=fwidth)
source = [mp.Source(src, component=mp.Er, center=source_center, size=source_size)]
,

The current results therefore do not seem to be accurate because of the $\Delta r$ error that is being introduced by this source.

Perhaps we need to modify this test to exclude $r=0$ in the line source?

cc @mochen4

@oskooi oskooi requested a review from mochen4 February 7, 2023 23:47
@stevengj
Copy link
Collaborator

stevengj commented Feb 8, 2023

This check is not reliable, because the user-specified amplitude function A(x) could be zero at r=0. But I don't know a good way to check that without sampling A at every point along the z axis?

(Also, note that for m=0 the phi and r components should be zero at r=0.)

@mochen4
Copy link
Collaborator

mochen4 commented Feb 8, 2023

As Steven pointed out, the source can be valid if the amplitude is 0 at $r=0$. The test looks good to me, though.

@mochen4
Copy link
Collaborator

mochen4 commented Feb 8, 2023

This check is not reliable, because the user-specified amplitude function A(x) could be zero at r=0. But I don't know a good way to check that without sampling A at every point along the z axis?

Don't we already have A at every point when we create src_vol to add the sources? Can we do the check here?

amps_array[idx_vol] = IVEC_LOOP_WEIGHT(s0, s1, e0, e1, 1) * amp * data->A(rel_loc);

@oskooi
Copy link
Collaborator Author

oskooi commented Feb 9, 2023

(Also, note that for m=0 the phi and r components should be zero at r=0.)

Added an additional check for this case.

Don't we already have A at every point when we create src_vol to add the sources? Can we do the check here?

Good suggestion. I have moved the check from add_volume_source to src_vol_chunkloop. With this modification, the case of a user-specified amplitude function is also included.

I tested these changes internally with a couple of tests. It seems to work correctly.

src/sources.cpp Outdated Show resolved Hide resolved
src/sources.cpp Outdated Show resolved Hide resolved
src/sources.cpp Outdated Show resolved Hide resolved
src/sources.cpp Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2023

Codecov Report

Merging #2392 (7ae6a22) into master (b86b476) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #2392   +/-   ##
=======================================
  Coverage   72.57%   72.57%           
=======================================
  Files          17       17           
  Lines        5166     5166           
=======================================
  Hits         3749     3749           
  Misses       1417     1417           
Impacted Files Coverage Δ
python/adjoint/filters.py 60.58% <0.00%> (-0.29%) ⬇️
python/geom.py 94.15% <0.00%> (+0.01%) ⬆️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants