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

removed amp_func argument to add_eig_src method for EigenModeSource #2381

Closed

Conversation

Arjun-Khurana
Copy link
Contributor

Reference issue: #2379 (comment)

@smartalecH
Copy link
Collaborator

cc @hammy4815

@hammy4815
Copy link
Contributor

Would this not prevent amp_func from being used at all in the case where an EigenModeSource lacks a DiffractedPlanewave? I'm not too familiar with how amp_func gets used under the hood, but in add_source, mp.Source adds a volume source using amp_func:

meep/python/source.py

Lines 153 to 154 in d370f1a

elif self.amp_func:
add_vol_src(self.amp_func, self.amplitude * 1.0)

However, the EigenModeSource overloads the add_source method. The only other place where I see it fed into the C++ backend is here:

meep/python/source.py

Lines 671 to 674 in d370f1a

if isinstance(self.eig_band, mp.DiffractedPlanewave):
add_eig_src(self.amp_func, diffractedplanewave)
else:
add_eig_src(self.amp_func)

Which is getting removed in this PR. But if it does get used somewhere else somehow, then I think this is fine as far as the new framework for add_source is concerned.

@Arjun-Khurana
Copy link
Contributor Author

Yeah this commit was just my quick and dirty solution for getting rid of the segfault in my example, something like this I think preserves the behavior in the case that an amp_func is supplied for an EigenModeSource:

if isinstance(self.eig_band, mp.DiffractedPlanewave):
    add_eig_src(self.amp_func, diffractedplanewave) if self.amp_func else add_eig_src(diffractedplanewave)
else:
    add_eig_src(self.amp_func) if self.amp_func else add_eig_src()

However I'm not sure an EigenModeSource should really ever have a user-specified amp_func, since I thought the point of this type of source is that the spatial profile is deduced from the waveguide structure using MPB. If that's the case, I think we can remove the reference to amp_func entirely in the overload of add_source, if not I believe the above logic should work.

@hammy4815
Copy link
Contributor

This will probably work, and maybe we pull it for now. However, I'm wondering if we should fix the bigger issue going on, i.e. why after the adjoint run is the eigenmode source set up incorrectly with regards to amp_func? If we can instead patch the memory leak, it might be more optimal of a solution.

@stevengj
Copy link
Collaborator

However I'm not sure an EigenModeSource should really ever have a user-specified amp_func, since I thought the point of this type of source is that the spatial profile is deduced from the waveguide structure using MPB.

The user-specified amp_func is multiplied by the MPB amplitude, e.g. if you want to add some Gaussian envelope to get a localized window of an eigenmode:

return (complex<double>(double(real(ret)), double(imag(ret))) * amp_func(p)) *

@smartalecH
Copy link
Collaborator

The issue isn't with the amp_func, it's the NULL pointer. So let's just use #2394.

@smartalecH smartalecH closed this Feb 9, 2023
]
add_eig_src = functools.partial(
sim.fields.add_eigenmode_source, *add_eig_src_args
)

if isinstance(self.eig_band, mp.DiffractedPlanewave):
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to change this, so let's punt to the simpler PR.

@smartalecH
Copy link
Collaborator

(we can always re-open if I'm wrong)

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.

4 participants