-
Notifications
You must be signed in to change notification settings - Fork 81
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
replaced bondgraph with networkx #1087
Conversation
for more information, see https://pre-commit.ci
Tests for converting to gmso will fail until we can push a PR with updates to the gmso conversion functions, but all tests pass on my local machine (with updated gmso routines). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1087 +/- ##
==========================================
- Coverage 90.64% 89.37% -1.27%
==========================================
Files 61 61
Lines 6240 6164 -76
==========================================
- Hits 5656 5509 -147
- Misses 584 655 +71 ☔ View full report in Codecov by Sentry. |
Did some digging and googling, I think the failing test probably memory related (known issue with networkx graph of complicated graph) |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes, and one major change if we deem it worthwhile to try to make the bondgraphs as fast as possible. If we want this connected_components change, I will open the PR in GMSO that will allow the 5 tests to pass in mBuild.
Okay, as soon as the CI get's figured out this is good to go. Can we make a change to mbuild/.github/workflows/CI.yaml, such as this line, to skip over that test? |
PR Summary:
Related to PR #1084
Basically, the bond graph is replaced with networkx.
PR Checklist