Validate that bend radius is not smaller than half the mode plane size #2053
Conversation
tidy3d/components/validators.py
Outdated
| @@ -396,3 +396,21 @@ def freqs_not_empty(cls, val): | |||
| return val | |||
|
|
|||
| return freqs_not_empty | |||
|
|
|||
|
|
|||
| def validate_mode_plane_radius(mode_spec: "ModeSpec", plane: "Box", msg_prefix: str): # noqa: F821 | |||
There was a problem hiding this comment.
I think it's fine (and perhaps better) to import these types and use directly here.
There was a problem hiding this comment.
That would be circular import as usually validators are imported in other components rather than the other way round. However, I don't know if this is the best place to put this function: it seems that everything else in this file creates an actual pydantic validator, while this is just a convenient method to use in a post-init validator.
There was a problem hiding this comment.
i think validators already imports from Box https://github.com/flexcompute/tidy3d/blob/develop/tidy3d/components/validators.py#L10 and ModeSpec doesnt import from validators, so is it circular?
There was a problem hiding this comment.
in any case, while separating the validators into their own file was maybe not a bad idea, functionally it's probably fine if this validator was just defined inside of simulation.py or wherever its used.
There was a problem hiding this comment.
Here's my thought:
- if the type annotation causes circular imports, we probably should be rethinking where to place the function in the first place. So in this case, should this just go in simulation.py for example? what's the benefit of separating it into validators.py when it just gets used in simulation things?
- If we want to keep it validators.py just for some separation, I think it's probably ok to import Box and ModeSpec at least the way it looks now, but it does mean we can't import validators from here those modules in the future.
so maybe overall I think it makes a bit more sense to just put this in simulation.py? or nearby (eg in new module base_sim/validators.py?)
There was a problem hiding this comment.
Oh interesting, sorry I was just thinking about it conceptually that this validators file would make sense to me to be at the top of the hierarchy.
Yea i'm also thinking about this out loud because I think it was never super well defined. Conceptually that makes sense, but also if these validators are going to use specific information about Box or ModeSpec components , they probably shouldnt be here. Maybe these more specific validators need to just go where they are used.
So I put it here because I need to use it both in Simulation and in ModeSolver.
Yea maybe in simulation.py or actually a new file base_sim/validators.py makes some sense? the ModeSolver would import from base_sim right?
There was a problem hiding this comment.
I think the original idea of the validators.py file is exactly for things that need to be imported in multiple files. Generally it makes sense to me that there may be such validators but not sure if a more fine-grained splitting would make more sense.
There was a problem hiding this comment.
yea it's just that there's some conflict between: being able to import these validators in many files vs. these validators being able to know about things in said files (eg. ModeSpec).
For now maybe my preference is make a new validators.py in sim_base and put this there (+ maybe other simulation specific validators that live in the current file?
There was a problem hiding this comment.
So I started doing this but actually I don't understand how this helps clarify the structure. What is the logic/difference between having validators in components/validators.py and components/base_sim/validators.py?
momchil-flex
force-pushed
the
momchil/validate_bend_radius
branch
from
9f5d1c5
to
4dfe57a
Compare
This was a bit annoying because I need to validate both mode sources/monitors and mode solver init, which have different fields needed for the validation ("size" for mode sources and monitors, "plane" for mode solver). So I couldn't write a native pydantic validator but made it a post-init instead. I also decided to put the validation in Simulation so I could point to the exact monitor/source index, but on the other hand maybe it's nicer to validate as soon as the object is created? Let me know what you think. Fixes #1299