-
Notifications
You must be signed in to change notification settings - Fork 517
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
[contrib] Use standard repn for incidence graph generation #2834
Conversation
…te definition of defaults
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.
This looks good! Just a couple minor questions that you can address or ignore.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2834 +/- ##
==========================================
- Coverage 87.19% 87.19% -0.01%
==========================================
Files 765 767 +2
Lines 88471 88548 +77
==========================================
+ Hits 77146 77211 +65
- Misses 11325 11337 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@@ -53,8 +53,7 @@ with fields for each of the four subsets defined by the partition: | |||
If any variables or constraints are unmatched, the (Jacobian of the) model | |||
is structurally singular. | |||
|
|||
.. doctest:: | |||
:skipif: not scipy_available or not networkx_available or not asl_available | |||
.. code-block:: python |
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.
Can we leave a comment as to why this is not being tested? Or better, why is this nondeterministic?
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.
Added a comment noting that the unmatched variables/constraints are not mathematically unique, and expanded on this in the text, noting that they depend on (a) how we identify incident variables and (b) the implementation of the matching algorithm.
"Regression test" on several examples:
*Leads to a different number of diagonal blocks in the block triangularization (between this PR and main) Here "Correct" means that, with this PR, the analysis leads us to the correct conclusion about what is wrong with the model. These were all run with IDAES 2.0. The comparison in #2816 uses a recent IDAES main, so the numbers reported in these two tables may not be directly comparable. |
Pypy test failure appears unrelated |
@andrewlee94 can you run IDAES tests with this branch? |
@Robbybp That fixed the known failures, but it does create two new ones, both related to the following:
These are coming up in the gas network tests (but I don't think that actually matters here). |
@andrewlee94 this should be fixed in IDAES/idaes-pse#1188 |
@Robbybp - Okay if we merge? |
@mrmundt Yep! |
Fixes
Variables with zero coefficients appear in incidence graphs
Summary/Motivation:
Incidence graphs where
0*x
appears as an edge can lead to incorrect results when analyzing singularity or generating decompositions.Changes proposed in this PR:
get_incident_variables
function, which accepts amethod
argument that can be used to switch between usingidentify_variables
andgenerate_standard_repn
IncidenceConfig
ConfigBlock
to consolidate specification of default options for incidence graph generationlinear_only
option for incidence graph generationLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: