Use default td.ModeSpec in mode monitors
#2058
Conversation
|
I'm ok with this but would rather put it in 2.8. But simply changing the base makes a mess, could you try cherry-picking your commit? |
yaugenst-flex
force-pushed
the
yaugenst-flex/monitor-modespec
branch
from
5cf8c61 to
bb3debd
Compare
|
Sure yeah, should be fine now. |
There was a problem hiding this comment.
i'm fine with it in 2.8 too. do we want to also add a mode_index: int = None and direction : str = None options? basically it would mean that we pre-select certain quantities from the data on the backend? and would make it more similar to ModeSource. If left None we just include all.
We could do this separately as well.
| @@ -279,7 +279,7 @@ class AbstractModeMonitor(PlanarMonitor, FreqMonitor): | |||
| """:class:`Monitor` that records mode-related data.""" | |||
|
|
|||
| mode_spec: ModeSpec = pydantic.Field( | |||
| ..., | |||
| default_factory=ModeSpec, | |||
There was a problem hiding this comment.
generally we just do ModeSpec() (no default factory). not sure if there's a substantial difference here, but i just dont know how the docs will handle default factories
There was a problem hiding this comment.
Should I change it or do we want to check how the docs look? Generally I would've thought that it's best to instantiate defaults (if they are classes) only on instantiation of the class, otherwise 1. you get object creation on import, which slows imports down and 2. it leads to shared state between different mode monitor instances. All new mode monitors will share the same default instance of mode_spec, and if you happen to make a change that modifies this default instance (let's say you write something into attrs), then this change will affect all other mode monitors too.
There was a problem hiding this comment.
i guess we could see how the docs looks, just might require some modification of this method that sets the docstring, we'd need to add handling for defaultfactory. Also cc'ing @daquinteroflex just so he knows since he's working on a sphinx plugin related to this.
There was a problem hiding this comment.
I took a stab at this in 1eafd9d which I think would correctly handle cases where we just use a class as a default factory. It wouldn't handle lambdas nicely, although if necessary I can also add support for that (by just printing the lambda definition).
i would suggest we do this separately if we decide to go for it. should probably avoid rushing that change, i'm also not so sure about it anymore... |
How do we feel about this? 😄