Allow field projections through a manually-specified medium #1291
Conversation
| @@ -11,6 +11,7 @@ | |||
| from .base import cached_property, Tidy3dBaseModel | |||
| from .mode import ModeSpec | |||
| from .apodization import ApodizationSpec | |||
| from .medium import MediumType | |||
There was a problem hiding this comment.
@tylerflex do you think importing from medium here could be problematic in the future? I'm not sure anymore if we ever defined the heirarchy of what's allowed to import from what.
There was a problem hiding this comment.
Shoot, i don't remember now but this is giving me Deja vu remembering some annoying refactorings. So I'm not sure. My feeling is we probably shouldn't import it here if possible. is there a way around it? Im on mobile and can't look now in detail
There was a problem hiding this comment.
One idea, to allow for frequency dependence, is a projection_index field that accepts either a scalar, or an array whose size is validated to match the size of monitor.freqs, so that the user can provide a constant or one value per frequency?
There was a problem hiding this comment.
For medium, my preference is to set it in the FieldProjector. For example, can we add an optional field projection_medium : MediumType = None, if it is set, we return it in the property FieldProjector.medium otherwise, we use the default FieldProjector.medium. This way we dont introduce any coupling between monitors and mediums that can trip us up down the line. @shashwat-sh @momchil-flex how does this sound?
There was a problem hiding this comment.
Well, it's definitely clunkier this way... But it sounds like we have to either live with it, or switch back to importing from Medium and hope this will not be an issue in the future?
There was a problem hiding this comment.
@tylerflex do you have a strong preference either way? Regarding importing MediumType, one consideration is how the structure might change in 3.0 or other future refactors? Conceptually, it does make sense to me that a Medium is more "fundamental" than a monitor, so medium.py should never depend on anything in monitor.py, but you never know...
There was a problem hiding this comment.
I suppose it's probably ok. I guess in 3.0 we might have a division between "physics / scene" (including mediums) and "simulation / experiment" (including monitors). So now that I think about it, it could be ok to import scene objects into simulation modules.
My knee jerk reaction was just to not couple things if possible and I specifically remember some coupling between medium and monitor before that was an issue, but now that I think about it, it probably makes sense to allow it assuming things all work and there aren't any other closed loops formed by this import.
There was a problem hiding this comment.
At one point we had discussed getting rid of client-side projections altogether, especially if the plan is to provide a standalone server-side projector. Is this still the goal? If so, maybe the clunkiness is tolerable since it's mostly field_projection.py that's clunky now?
There was a problem hiding this comment.
It also seems more annoying for the user if it's not taking medium directly. Let's go with medium.
|
Squash and ship as far as I'm concerned. Could you also |
bc54660 to
8684c7c
Compare
Backend PR: https://github.com/flexcompute/tidy3d-core/pull/474
Addresses this issue.