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

Execution Graph and Computation Optimization #27

Merged
merged 7 commits into from
Jan 30, 2025
Merged

Conversation

yuefan98
Copy link
Owner

Execution Graph

  • Added support to graph-based function evaluation (Thanks @andersonjacob for the source code)
  • Added more tests to cover the changes
  • Added speed benchmark for the graph on documentation

Computation Optimization

  • Vectorized calculation in circuit elements for fast computation

New Circuit Elements

  • Added support to nonlinear RC with constant phase element: (RCDQ, RCDQn), and (RCSQ, RCSQn)
  • Added more tests to cover the changes

Documentation

  • Added Release Notes

General Code Improvement

  • Minor code improvement for better handling
  • Bumped version to v0.2
  • Updated requirement.txt and setup.py

Test Fixes: update expected output in NLEIS tests to reflect correct units
* Enabled circuit processing and circuit evaluation using execution graph
* Updated and passed all the corresponding tests
* Updated requirements.txt and setup.py
* Bumped up version number
* Fixed some docstrings
* Added speed benchmark for graph-based function evaluation to examples
* Added release notes
* Improved CircuitGraph class docstrings
@coveralls
Copy link
Collaborator

coveralls commented Jan 15, 2025

Pull Request Test Coverage Report for Build 12875811319

Details

  • 474 of 483 (98.14%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 96.404%

Changes Missing Coverage Covered Lines Changed/Added Lines %
nleis/nleis_elements_pair.py 339 348 97.41%
Totals Coverage Status
Change from base Build 12246244250: 0.4%
Covered Lines: 1555
Relevant Lines: 1613

💛 - Coveralls

@yuefan98
Copy link
Owner Author

Hi @andersonjacob,

Thanks for your excellent code on CircuitGraph. I made small edits to it and it worked seamlessly with my code. With some benchmark results, the graph-based evaluation can at least 3X speed up the function evaluation. Surprisingly it does not introduce much performance improvement when the circuit is matrix-driven and the eval() method can be faster sometimes (not explicitly shown in the benchmark results). Without too much change to the main code, I also made graph-based evaluation an option to the eval() method rather than directly substituting it. With said said, I would like to invite you to review the changes I made so that I can address potential issues that I haven't considered yet.

Main changes to the CircuitGraph function include:

  • Supports the parsing of difference element (d)
  • Added __eq__ to enable the comparison of the two graphs through node and edge comparison.
  • I also realized that I have make __call__ equivalent to compute_long. And their implementations are kind of duplicated. I might probably make it to return impedance directly, but for now, they work just fine.

nleis/fitting.py Outdated
dnode = f"d{self.dnum}"
self.dnum += 1
self.graph.add_node(dnode, Z=circuit_elements["d"])
for elem in pd_elem:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if statements are really only needed to get the number to use. You could simplify it a lot with

if operator == "p":
  nnum = self.pnum
  self.pnum += 1
elif operator == "d":
  nnum = self.dnum
  self.dnum += 1

node = f"{operator}{nnum}"
self.graph.add_node(node, Z=circuit_elements[operator])
for elem in pd_elem:
  elem = self.add_series_elements(elem)
  self.graph.add_edge(elem, node)
parsing_circuit = parsing_circuit.replace(pd, node)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spending time reviewing my implementation. I agree that this will make the code cleaner and simpler. I will make the changes accordingly.

@andersonjacob
Copy link

One thing that may make things easier/better for the long term, but not for the short term. Your code depends on impedance.py so if the graph where merged there then you could inherit from it and just override what you needed and keep the rest the same. Then you don't have to implement things twice.

@yuefan98
Copy link
Owner Author

One thing that may make things easier/better for the long term, but not for the short term. Your code depends on impedance.py so if the graph were merged there then you could inherit from it and just override what you needed and keep the rest the same. Then you don't have to implement things twice.

Thanks for the note! That is exactly my long-term goal for the nleis.py project. Once the nleis.py has been stably running for a well, I will make sure it inherits impedance.py as much as possible so I can either integrate it to impedance.py or make it as a simple add-on to impedance.py

* simplify the code for parse_circuit
* Update compute methods to return impedance value
* add networkx dependency to environment.yml
@andersonjacob
Copy link

The code itself looks very similar to what I would expect. I haven't run anything so I can't verify that it works as one would expect, but I am assuming that your tests will cover most all of that.

I think it looks good.

@yuefan98
Copy link
Owner Author

Yes, the tests cover most of the essential use cases and make sure the graph-based calculation is equivalent to the eval()-based calculation. With all the tests passed, the code should work as expected.

Thank you so much for your contribution and I will merge the PR soon. 🚀

@yuefan98 yuefan98 merged commit 27542ba into main Jan 30, 2025
6 checks passed
@yuefan98
Copy link
Owner Author

@all-contributors please add @andersonjacob for code and review.

Copy link
Contributor

@yuefan98

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

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.

3 participants