pydantic anti patterns

[tylerflex]

Things we do that maybe we shouldn't.

Setting an attribute in the model.

class Model:
    data:Any = None


    def some_method(self):
        self.data = something

What if the user specifies data2?

Properties and attributes

class Model:

    data: Any

    @property
    def data(self):
         return something

[momchil-flex]

We never do 2. directly, but what we have in the sources is something like this:

class PlanarSource(Source, ABC):
    """A source defined on a 2D plane."""

    @property
    def injection_axis(self):
        """Injection axis of the source."""
        return self.size.index(0.0)

    @injection_axis.setter
    def injection_axis(self):
        raise ValueError("PlanarSource injection axis is defined by the axis normal to the plane.")

class DirectionalSource(Source, ABC):
    """A Source defined with a direction of propagation."""

    direction: Direction
    injection_axis: Axis

class ModeSource(PlanarSource, DirectionalSource):
    """Injects current source to excite modal profile on finite extent plane."""

I feel like this is almost kosher, because there's no way (that I can think of) that it can produce unexpected results. That said it's probably not best practice. It's just that the alternatives seem to make code reuse (e.g. in the plotting of the arrows) harder.

Note also that currently we don't have any DirectionalSource that is not a PlanarSource. One example would be a TFSFSource, when we have that eventually.

Regarding 1, we only do that in validators, right? I wonder if, again in the interest of simplified API and internal code, doing something like this can't be considered OK if we make sure to validate for the extra data in the subclass:

class Model1:
    data1: Any = None

class Model2(Model1):
    data2: Any = None

    pydantic.validator("data2")
    def validate_data2(cls, val, values):
         values["data1"] = something

    pydantic.validator("data1")
    def validate_data1(cls, val):
        raise ValueError("'data1' is implicitly set by 'data2' in 'Model2' models.")

This could be the approach in e.g. PolySlab w.r.t. slab_bounds and center, size.

[tylerflex]

Regarding 1, maybe the best strategy is to use properties instead of through validators or setting somewhere else in code.

class Model:
    data:Any = None
    def some_method(self):
        self.data = something

becomes

class Model:
    @property
    def data(self):
        return something

[momchil-flex]

Well this is certainly a good approach within the limited scope of this toy example, but the point is what happens when other models subclass from Model1 and do want to have data1 as a field. Think of slab_bounds of PolySlab and center, size of the base Geometry.

[tylerflex]

Yea I guess this approach I put was with things like Job.task_id in mind. We initialize it as None in the model but then set it using the task_id returned from web in Job.upload(). Maybe task_id is better left as a property` here.

For geometry, I'm not so sure. We might need to rewrite the polyslab to use a center + size approach by default instead of by bounds for this to be consistent.

[tylerflex]

Regarding 2, seems a bit tricker, maybe we need to refactor a little more to make it work. I'll think about it some more.

[momchil-flex]

Yea I guess this approach I put was with things like Job.task_id in mind. We initialize it as None in the model but then set it using the task_id returned from web in Job.upload(). Maybe task_id is better left as a property` here.

Yeah that should definitely be set up like that.

For geometry, I'm not so sure. We might need to rewrite the polyslab to use a center + size approach by default instead of by bounds for this to be consistent.

Maybe, I'm just wondering if it's not OK using the validators as I proposed above in the interest of simpler API.

Regarding 2, seems a bit tricker, maybe we need to refactor a little more to make it work. I'll think about it some more.

Yeah give it some thought, another strange thing there is that the DirectionalSource expects to be composed with or inherited in another class that defines angle_theta and angle_phi (those are used in DirectionalSource.plot but are not explicitly defined in DirectionalSource). This is because they are properties in ModeSource and fields in the other sources.

[tylerflex]

Revisiting this a bit because why not:

• seems like pattern 2 is not longer present in the sources.py. So in my mind we can forget it for purposes of this issue.
• Regarding 1, maybe the best way to handle this is simply to make the field a PrivateAttr when it comes up. That way we simply shouldn't have to worry about users setting _data (unless they are up to no good).

class Model:
    _data: DataType = PrivateAttr(None)


    def some_method(self):
        self._data = something

[momchil-flex]

Yeah, that works if possible. But the main issue with this has been setting something in a subclass that is a proper field in the parent class (setting length from slab_bounds in PolySlab).

[tylerflex]

In this case, potentially the _length could be private (and used for Slab calculations) and then Cylnder.length and PolySlab.slab_bounds could set this private attribute in their validators.