Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use execution graph and add additional fitting methods #308

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andersonjacob
Copy link

I'll just get this out there early. This isn't a small change. I refactored the all the computation of the impedance for a circuit. This eliminates all the string building and calls to eval(...) in favor of an execution graph. This makes it easier to debug what is going on. My original goal was to tweak/improve the optimization, but once I ran into debugging difficulties, it all got changed.

From the outside things look almost identical with a few additional fitting options, but under the covers a lot of things have changed. All the tests + the new ones are passing, and the new DAG based circuit evaluation could open the door for more advanced things like parallel execution for large problems.

As an example, the circuit string: R0-p(R1,C1)-p(R2-Wo1,C2) is parsed into a graph that looks like:
image
where you can see how the different nodes depend on the impedance of other nodes. The computation then proceeds from left to right, with the outputs of each node becoming inputs to successor nodes.

This visualization can be had with this simple example code:

g = CircuitGraph("R0-p(R1,C1)-p(R2-Wo1,C2)")
f, ax = plt.subplots(figsize=(6,6), layout="constrained")
g.visualize_graph(ax=ax)

This is a large change; I know that. If the changes are valuable, but splitting them up into more manageable chunks is desirable, I'm open to exploring that.

@andersonjacob
Copy link
Author

As I go through the issues, this fixes #163 by providing an option to return the estimated covariance matrix.

@mdmurbach
Copy link
Member

This is a pretty great set of changes, thanks @andersonjacob. It is a little challenging to work through all the logic, code quality, etc. changes in one big PR, but at a first glance the circuit graph idea seems like a good way to avoid eval and the visualization is neat. It'll take me a little time to work through the actual code/wrap my head around the new logic. Any limitations or breaking changes you can think of (will try running the tests locally since the GitHub Actions are failing for linting and skipping the tests)?

@BGerwe might also find this interesting as well.

@andersonjacob
Copy link
Author

andersonjacob commented Sep 12, 2024

@mdmurbach

Sorry, I'm just circling back to this; I've been an a conference all week. I totally understand that this is a really large change in the internals of the package and it will (should) take some time to go over.

  • I think I can have the linting errors cleaned up really easily. I mistakenly assumed that Ruff would format code well to pass the linting, but it didn't. I am sure that is an easy fix.
  • The new code shouldn't have any limitations in comparison to the old eval based code. It should produce equivalent results in all cases (that was the idea in any case and is true for all the circuits I've checked, but I don't have any formal proof). I want to say it should be faster, but I have zero evidence to back that up at this point.
  • The circuits interface is backwards compatible and so is the fit_circuit function. But there are several functions which are gone all together, and to the extent that they were used within this code base I've fixed them to use the CircuitGraph. If an end user were directly using a function that has been removed and not using fit_circuit or BaseCircuit.fit, then their code would break with these changes.
  • One difference which is backward compatible but which may not be obvious. The CustomCircuit is now just an alias to BaseCircuit. CustomCircuit didn't add any functionality to the BaseCircuit and the functions in BaseCircuit depended on the self.circuit member variable which was not actually defined in its constructor. So I added an optional circut parameter to the BaseCircuit constructor and made the CustomCircuit just an alias to it. Old code will totally work with the new alias, but it does mean that you can create do cc = CustomCircuit() and it will not complain at all. It is obviously a very boring ciruit as it has zero elements in it, but is isn't an error either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants