IDAES/idaes-pse

Add parallel variables/constraints to `report_numerical_issues`

Closed this issue · 3 comments

Robbybp commented

Right now, display_near_parallel_* methods are considered "advanced diagnostic techniques" and only suggested if report_numerical_issues finds no issues. However, I would argue that they should be considered "numerical diagnostics" as they run quickly, give concise output (for an appropriately tight tolerance), and don't depend on external solvers. I propose we add them to the "numerical warnings" section of the diagnostics toolbox. I am happy to open a PR if others agree.

andrewlee94 commented

@Robbybp I think they were added as "advanced" methods as the earlier implementations at least were rather slow (especially for large models). I don't think I've tried your more efficient implementation on a large model yet however, so they might be OK now.

We should do some quick checks to be sure, but if these methods are not sufficiently fast I do not see a reason for them not to become part of the main numerical checks.

Robbybp commented

In a recent test on a 10k variable/constraint problem, the bottleneck is constructing the PyomoNLP/Jacobian (which we could amortize away with a little work):

Identifier                        ncalls   cumtime   percall      %
-------------------------------------------------------------------        
parallel-constraints                   1     3.996     3.996  100.0         
                    -----------------------------------------------         
                    dot-prods          1     0.359     0.359    9.0
                    get-jac            1     3.213     3.213   80.4  
                    get-vectors        1     0.397     0.397    9.9
                    sort-by-nz         1     0.022     0.022    0.6           
                    other            n/a     0.006       n/a    0.1
                    ===============================================
===================================================================

Let me know the results if you get around to benchmarking.

andrewlee94 commented

If the time is mainly taken in getting the Jacobian, then we should go ahead and integrate this. All the other numerical methods already have an optional argument for the Jacobian to be passed in if it has already been generated, and the _collect_numerical_warnings method does this once up front and passes it on to all the methods that need it. So, we just need ot do the same here.

If you want to open a PR to do this, you are more than welcome to do so.

  • 👍1