Support multi-letter elements, make adding new elements easier. #82

nealde · 2020-02-20T08:57:17Z

confirmed working multi-letter elements, and cleaned up the element inheritance so that addition of a new element only requires adding it to the file. The function names are added to a dictionary where the function call is the key, i.e. R(a,b) is represented as 'R'. Downstream code required no modification, as these were added to the globals() dictionary upon import.

The tests were updated so that the inputs and expected outputs are held in dictionaries so that order is no longer important.
The code to extract the elements from the circuit was altered significantly, and a test of this function has been added. It has been demonstrated to support arbitrarily high integers and still properly parses them.
The new element, RR, is a resistor whose value is multiplied by 100. I've verified that, when fit, the parameter value is 100-fold smaller than the expected R value, and that the fits are identical. It's just a placeholder, feel free to delete it whenever.
There's also a test that enforces that elements are only allowed to be all-capital letters, so no integers or lower-case letters allowed, or the test will fail.

…nheritance so that addition of a new element only requires adding it to the file. The function names are added to a dictionary where the function call is the key, i.e. R(a,b) is represented as 'R'. Downstream code required no modification, as these were added to the globals() dictionary upon import. > The tests were updated so that the inputs and expected outputs are held in dictionaries so that order is no longer important. > The code to extract the elements from the circuit was altered significantly, and a test of this function has been added. It has been demonstrated to support arbitrarily high integers and still properly parses them. The new element, RR, is a resistor whose value is multiplied by 100. I've verified that, when fit, the parameter value is 100-fold smaller than the expeccted R value, and that the fits are identical.

…hat is what was holding up travisCI

nealde · 2020-02-20T09:24:41Z

I officially have no idea why Travis-CI is failing at the flake8 step after seeming to succeed. Very strange.

…the error numbers meant before

coveralls · 2020-02-20T10:26:41Z

Pull Request Test Coverage Report for Build 241

237 of 255 (92.94%) changed or added relevant lines in 7 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.2%) to 91.56%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
impedance/models/circuits/fitting.py	38	41	92.68%
impedance/models/circuits/circuits.py	166	181	91.71%

Files with Coverage Reduction	New Missed Lines	%
impedance/models/circuits/fitting.py	1	96.88%

Totals
Change from base Build 231:	0.2%
Covered Lines:	857
Relevant Lines:	936

💛 - Coveralls

mdmurbach · 2020-02-20T14:20:44Z

Awesome! Thanks for such a quick turn around 🚀 I’ll take a dive into this sometime today. The use of a dictionary and globals should make it a lot cleaner and easier to add a new element! Thats awesome.

Any technical reason why elements would need to be all caps? From the electrochemistry side I think we need to find a more standard naming scheme, but the way I was thinking about it was as camel case (Wa for Warburg, Wo for Warburg open, etc.). @BGerwe or @lktsui any thoughts/wishes for how we should name the current elements for clarity?

nealde · 2020-02-20T14:43:17Z

As far as all-caps is concerned, I just figured that they were all caps now, and that that trend might continue. Enforcing just upper/lower case (mostly no numbers or special characters) wouldn't be much more difficult.
I don't think there's a technical reason they can't even be full on words - it handles the difference between R and RR because, since everything is in a dictionary now, it hashes the whole input, and the inputs are parsed based on having an integer following them. So, from an implementation standpoint, I think the sky is the limit. I wouldn't want to have to type that many characters in the circuit string, but you should have total flexibility.

mdmurbach · 2020-02-20T16:33:11Z

impedance/models/circuits/circuits.py

+            return eval(buildCircuit(self.circuit, frequencies,
+                                     *self.parameters_,
+                                     constants=self.constants, eval_string='',
+                                     index=0)[0])


Instead of updating the globals() up top, I think we can just pass the circuit elements dictionary (which is a fantastic idea in terms of not importing a bunch of individual elements) directly to the eval function. So it'd just be eval(....., circuit_elements). The downside of this is that we either have to add s, and p to the circuit elements dict or pass them in as well, but I think it might be better than updating the globals in the whole module in terms of readability/namespace?

ooohh I had forgotten about the ability to pass a dictionary / namespace into eval, that would make it more readable for sure. The upside of this implementation was that it was less modified code, but the readability has definitely suffered for it. I'll test this out and see how it works.

mdmurbach · 2020-02-20T16:39:51Z

impedance/models/circuits/elements.py

+non_element_functions = ['element_metadata',
+                         'initial_state',
+                         'non_element_functions',
+                         's',


What do you think of removing s and p from here and treating them as elements? (for easier importing)

I think that's a good idea, I've almost got all of the tests passing using that. We just have to make exceptions for them in the "expected outputs given an input" test

to be clear - not planning on pushing it right away, so keep picking through the code. I just wanted to make sure there weren't any clear roadblocks

mdmurbach · 2020-02-20T16:49:50Z

impedance/models/circuits/elements.py

+                         'non_element_functions',
+                         's',
+                         'p',
+                         'K',


K is also an element that is used for the LinKK method, so I think it can also be added here too

mdmurbach · 2020-02-20T16:51:14Z

impedance/tests/test_circuit_elements.py

+                          (0.07107755021941357-0.03427465211788068j),
+                          (0.00199550459845528-0.0019939172581851707j)],
+                    'T': [(1.00041-0.00837309j),
+                          (0.0156037-0.114062j),


added a test for K, ignoring s, and p, and using the num_params attribute of the function wrapper instead of an inputs dictionary

'K': [(1e-1-2e-5j), (9.6154e-02-1.9231e-02j), (2.5e-6-5e-4j)], 'RR': [10, 10, 10]} input_vals = [0.1, 0.2, 0.3, 0.4] for key, f in circuit_elements.items(): if f.__name__ not in ['s', 'p']: num_inputs = f.num_params val = f(input_vals[:num_inputs], freqs)

mdmurbach · 2020-02-20T16:51:53Z

impedance/models/circuits/fitting.py

-    if element not in allowed_elements or len(element) != 1:
+    allowed_elements = circuit_elements.keys()
+    if element not in allowed_elements:
+        print(element, allowed_elements)
        raise ValueError
    else:
        return eval(element)


have to pass circuit elements to this as well.

mdmurbach · 2020-02-20T16:56:57Z

Made some quick comments. This is super awesome flexibility. I really like the way elements.py can be added to without copying anything new to an import statement in a bunch of different files 😄 🚀 My main suggestion would be to pass the new circuit_elements dictionary to eval directly (just feels better to me, but if it's bad for some reason, I'd be interested in why).

Learned something new about globals today, thanks 😃

nealde · 2020-02-20T16:58:42Z

I think that's an awesome idea, I almost have it working locally. Thanks for the K test, I'll import it there and add it to the list!
Yeah, the globals thing is a pretty neat trick!

…ts dictionary so the globals() namespace stays un-modified. This should help readability. Additionally, s, p, and K are now considered 'elements', and the circuit_elements dictionary is implemented for linKK methods

BGerwe · 2020-02-20T18:20:14Z

Looks like you two have already touched on the points I wanted to discuss. I'm really excited about how much easier it will be to add new elements. Some people I wanted to start using impedance.py were hesitant because it didn't have the elements they wanted and adding new ones was such a pain.

The Codacy review is failing because of using eval instead of ast.literal_eval. I played around with that one day and found that ast.literal_eval didn't work as a drop-in replacement of eval, but maybe it will with the dictionary implementation? Alternatively, @mdmurbach could consider ignoring issues from that code pattern if he's not concerned about the security of eval.

mdmurbach · 2020-02-20T19:11:39Z

@BGerwe it looks like literal_eval won't work because "The string or node provided may only consist of the following Python literal structures: strings, numbers, tuples, lists, dicts, booleans, and None." I had meant the check_and_eval function to basically be the safety net on eval so we could turn off the codacy check

BGerwe · 2020-02-20T19:43:51Z

That makes a lot of sense. I just wanted to bring attention to the cause of those nasty red X's

nealde · 2020-02-21T07:25:06Z

I think the fact that we are now passing a dictionary to eval makes it very safe - nasty functions would have to make their way into that dictionary in order to run.

I won't be online for a few days, so if you guys want to merge this, feel free! If there are any issues, I'd be happy to help troubleshoot this week!

mdmurbach · 2020-02-21T14:39:54Z

I agree @nealde and turned off the Codacy rule yesterday, but I don’t think it will update here. If you’re good with this @BGerwe, I think we can merge it in. I think we want to rename some of the elements (Warburg and Porous electrode, in particular) but I’ll make another issue for that

BGerwe · 2020-02-21T15:43:40Z

Sounds great!

mdmurbach · 2020-07-21T23:26:09Z

@all-contributors please add @nealde for code

allcontributors · 2020-07-21T23:26:17Z

@mdmurbach

I've put up a pull request to add @nealde! 🎉

nealde requested review from mdmurbach and BGerwe February 20, 2020 08:57

Neal Dawson-Elli added 2 commits February 20, 2020 10:08

cleaned the code up a bit and made it more flake8 compliant in case t…

2332487

…hat is what was holding up travisCI

removed an f string in case that was causing the issue

8d366a2

okay, hopefully this fixes flake8 now. I just didn't understand what …

9f2fdfe

…the error numbers meant before

mdmurbach reviewed Feb 20, 2020

View reviewed changes

BGerwe approved these changes Feb 21, 2020

View reviewed changes

BGerwe merged commit a49a19d into master Feb 21, 2020

mdmurbach mentioned this pull request Feb 21, 2020

Consistent naming scheme for circuit elements #83

Closed

mdmurbach mentioned this pull request Mar 13, 2020

Enable multi-character circuit elements #81

Closed

BGerwe deleted the feature/enable-multi-char-elements branch March 16, 2020 20:58

allcontributors bot mentioned this pull request Jul 21, 2020

docs: add nealde as a contributor #124

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi-letter elements, make adding new elements easier. #82

Support multi-letter elements, make adding new elements easier. #82

nealde commented Feb 20, 2020

nealde commented Feb 20, 2020

coveralls commented Feb 20, 2020 •

edited

mdmurbach commented Feb 20, 2020

nealde commented Feb 20, 2020

mdmurbach Feb 20, 2020

nealde Feb 20, 2020

mdmurbach Feb 20, 2020

nealde Feb 20, 2020

nealde Feb 20, 2020

mdmurbach Feb 20, 2020

mdmurbach Feb 20, 2020

mdmurbach Feb 20, 2020

mdmurbach commented Feb 20, 2020

nealde commented Feb 20, 2020

BGerwe commented Feb 20, 2020

mdmurbach commented Feb 20, 2020

BGerwe commented Feb 20, 2020

nealde commented Feb 21, 2020

mdmurbach commented Feb 21, 2020

BGerwe commented Feb 21, 2020

mdmurbach commented Jul 21, 2020

allcontributors bot commented Jul 21, 2020

Support multi-letter elements, make adding new elements easier. #82

Support multi-letter elements, make adding new elements easier. #82

Conversation

nealde commented Feb 20, 2020

nealde commented Feb 20, 2020

coveralls commented Feb 20, 2020 • edited

Pull Request Test Coverage Report for Build 241

💛 - Coveralls

mdmurbach commented Feb 20, 2020

nealde commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdmurbach commented Feb 20, 2020

nealde commented Feb 20, 2020

BGerwe commented Feb 20, 2020

mdmurbach commented Feb 20, 2020

BGerwe commented Feb 20, 2020

nealde commented Feb 21, 2020

mdmurbach commented Feb 21, 2020

BGerwe commented Feb 21, 2020

mdmurbach commented Jul 21, 2020

allcontributors bot commented Jul 21, 2020

coveralls commented Feb 20, 2020 •

edited