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

Add aux field monitors #2309

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add aux field monitors #2309

wants to merge 1 commit into from

Conversation

caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Mar 12, 2025

Adds monitor class AuxFieldTimeMonitor and corresponding data classes AuxFieldTimeData and AuxFieldTimeDataset. Currently it is modeled after FieldTimeMonitor -- you specify some aux field components to download, and it puts those in the corresponding data class. The default is no aux field components. Any field components which don't correspond to nonlinearities which are present in the region / material / simulation are just stored as 0.

Todo:

  • Add frontend unit tests
  • Finalize api
  • Maybe validate against requesting nonlinear aux field components for nonlinearities not present in the simulation. This was the purpose of the function _aux_fields in Simulation.

@caseyflex caseyflex force-pushed the casey/nonlinearmonitors branch 2 times, most recently from fb057ec to 65d0934 Compare March 12, 2025 18:10
@caseyflex caseyflex marked this pull request as draft March 12, 2025 18:10
@caseyflex caseyflex requested a review from momchil-flex March 12, 2025 18:10
@caseyflex caseyflex force-pushed the casey/nonlinearmonitors branch from 65d0934 to ed6377a Compare March 13, 2025 09:03
@caseyflex caseyflex requested a review from yaugenst-flex March 13, 2025 09:12
@caseyflex caseyflex marked this pull request as ready for review March 13, 2025 09:14
@caseyflex caseyflex force-pushed the casey/nonlinearmonitors branch from ed6377a to 0792e87 Compare March 17, 2025 11:40
Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Thanks @caseyflex this looks really solid so code-wise I don't have any comments, more of a general question - is this something that we expect users to use or is it mostly for internal purposes? Also, what would happen if we were to add support for more auxiliary fields in the future? I wonder if these fields could just be stored in the regular FieldTimeData?

@caseyflex
Copy link
Contributor Author

Thanks @caseyflex this looks really solid so code-wise I don't have any comments, more of a general question - is this something that we expect users to use or is it mostly for internal purposes? Also, what would happen if we were to add support for more auxiliary fields in the future? I wonder if these fields could just be stored in the regular FieldTimeData?

Yeah, I think users will use it. Already knowing the free-carrier density when using TwoPhotonAbsorption nonlinearity is quite useful.

Regarding future aux fields -- I was thinking that we could just add more fields to AuxFieldTimeData every time we need to add a new field. Would that cause backwards compatibility issues? This is related to the last point of just using FieldTimeData -- in principle we could use that here too, but we would need to add fields to it which would clutter that dataset (currently it has Ex, Ey, Ez, Hx, Hy, Hz and nothing else). Also electromagnetic field data is treated specially in other places; you can compute poynting vectors, overlaps, and other things that don't make sense for aux fields.

@yaugenst-flex yaugenst-flex self-requested a review March 18, 2025 14:23
@yaugenst-flex
Copy link
Collaborator

Regarding future aux fields -- I was thinking that we could just add more fields to AuxFieldTimeData every time we need to add a new field. Would that cause backwards compatibility issues?

No I think that'd be fine as long as all fields are optional.

Also electromagnetic field data is treated specially in other places; you can compute poynting vectors, overlaps, and other things that don't make sense for aux fields.

Yeah I agree. Let's keep it like this for now.

@caseyflex caseyflex force-pushed the casey/nonlinearmonitors branch from 0792e87 to 15b1e4d Compare March 19, 2025 10:12
@caseyflex caseyflex force-pushed the casey/nonlinearmonitors branch from 15b1e4d to 22ed222 Compare April 1, 2025 09:06
@caseyflex caseyflex force-pushed the casey/nonlinearmonitors branch from 22ed222 to 22a4783 Compare April 1, 2025 09:06
Nfx=lambda dim: +1,
Nfy=lambda dim: +1,
Nfz=lambda dim: +1,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I took another look at this PR to see if the symmetry expansion, and grid locations would be correctly handled and I'm pleasantly surprised that this seems to have been set up in a pretty expandable way previously! :D I was worried about it.

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.

3 participants