BYUCamachoLab / simphony

A simulator for photonic integrated circuits.
https://simphonyphotonics.rtfd.io
Other
117 stars 35 forks source link

Model objects should take a name option in the constructor #93

Closed Andeloth closed 1 year ago

Andeloth commented 1 year ago

Model objects have a setter to set their name here, and a property for it here

https://github.com/BYUCamachoLab/simphony/blob/683843ccec7d59f39d6e4724f1e65def362349ac/simphony/models.py#L363C9-L363C9

but they don't take an optional name argument in their constructor, so it requires 2 calls to set the name of a model object at the moment. Should add the optional name kwarg to the constructor.

sequoiap commented 1 year ago

@Andeloth Is there a reason to be able to set names on models? The reason it was implemented was simply for giving unique names in the NetworkX diagrams that we generate while still remaining compatible with giving circuits names, since they inherit from Model. What is the use case in naming a model explicitly? The only one I can potentially think of is for display in those NetworkX graphs, is that what you want it for?

Andeloth commented 1 year ago

yep

sequoiap commented 1 year ago

Ok. Go ahead and add that to the initializer and then link the commit number back here, no need to make a whole pull request but I would like to track where changes were made

Andeloth commented 1 year ago

What would be a better way to implement this?

  1. Make the Model class always hijack the name keyword argument if supplied. This way the child classes of Model don't need to call super().init(name) so the people implementing new models don't need to worry about it but in return the name keyword argument in the child class constructor call will always be none, i.e.

    from simphony.models import Model
    class TestModel(Model):
    ocount=1
    def __init__(self, name:str = None):
        self.testname = name
        print(self.testname)
    x = TestModel("test")

    prints None.

  2. Change all the current classes in simphony to take the name kwarg and call super().__init_(name).

I think the 1st is a more elegant solution but it might introduce unexpected behavior, option 2. is best. What do you think @sequoiap ?