Allow for adding of user defined circuit elements

ECSHackWeek / impedance.py

A Python package for working with electrochemical impedance data

MIT License

212 stars 103 forks source link

Allow for adding of user defined circuit elements #219

Closed mdmurbach closed 1 year ago

mdmurbach commented 2 years ago

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

rgasper commented 2 years ago

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?

mdmurbach commented 2 years ago

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?

BGerwe commented 2 years ago

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.

rgasper commented 2 years ago

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 commented 2 years ago

check the PR, new overwrite behavior implemented

mdmurbach commented 1 year ago

Implemented by #196