impedance.py icon indicating copy to clipboard operation
impedance.py copied to clipboard

Allow for adding of user defined circuit elements

Open mdmurbach opened this issue 1 year ago • 5 comments

https://github.com/ECSHackWeek/impedance.py/pull/196

@rgasper has an interesting idea that would make requests for adding new elements unnecessary, e.g. https://github.com/ECSHackWeek/impedance.py/issues/184 and https://github.com/ECSHackWeek/impedance.py/issues/130

mdmurbach avatar Sep 18 '22 20:09 mdmurbach

Ah I didn't have my github notifications configured very well so I missed all those comments in April. I'll quickly go through and clean my PR up some time this week. The unintended behavior of not being able to redefine any element you've created w/o restarting the python VM is definitely sub-optimal.

Resolution Options:

  • create a set of static circuit components which you're not allowed to redefine - probably just the ones currently in the library. Then any new component could be freely defined and redefined at runtime. I think raising a custom CannotOverwriteCircuitException would also be helpful to make that behavior clear to the users.
  • Emit a python warnings.warning when redefining a circuit. Then the user can decide whether to care about warning or not.

@mdmurbach any thoughts on those behavior options, or which components should be in that "un-redefinable" list?

rgasper avatar Sep 26 '22 14:09 rgasper

No worries, glad you're still on board, think it's a cool addition!

I agree that not being able to redefine elements without restarting the python kernel is sub-optimal so allowing overwriting (removing the if statement) makes sense to me still. You bring up a good point about how to handle overwriting though and I think it makes sense to me that someone could rewrite the base functions if they wanted (I think it would be challenging to determine what is the base non-changeable set -- is it just what we have included in the package? just the R, L, C basic elements? etc.) but we should make it really clear when they do overwrite something already defined. The warning could work or you could also have the Exception, but provide a overwrite=True in the @element decorator if we want something a little more strict than the warning?

@BGerwe any thoughts?

mdmurbach avatar Sep 28 '22 04:09 mdmurbach

I really like the idea of user defined elements! But I am leaning towards not allowing folks to overwrite the existing circuit components. IMO, if someone is going to write a new element, it's trivial to name it something other than the existing elements. If there are errors in the existing ones we should fix them, but overwriting them seems like a can of worms.

BGerwe avatar Sep 29 '22 03:09 BGerwe

I agree with that interface. If these are generally agreed upon correct elements, it makes sense to put up at least a small barrier towards rewriting them. It's not really possible to permanently block people from overwriting the predefined elements in python since we don't have private or static variables - they could always screw around with the circuit_elements dictionary directly, but I'll at least make them define an overwrite=True to do so in the intended interface

rgasper avatar Sep 30 '22 00:09 rgasper

check the PR, new overwrite behavior implemented

rgasper avatar Sep 30 '22 00:09 rgasper