Clean up identify_variables / identify_mutable_parameters; deprecate SimpleExpressionVisitor

[jsiirola]

Fixes # .

Summary/Motivation:

This cleans up the implementation of identify_variables to simplify the implementation, improve efficiency, and improve robustness of the named expression cache:

• Reduce redundancy in the implementation
• Avoid allocating data structures for caching named_expressions unless the cache is defined and a named expression is actually encountered
• Improve the cache robustness, including robustly handling cache invalidation if the named expressions have changed.

In addition:

• identify_mutables_parameters is moved to build on identify_variables (by deriving from the IdentifyVariablesVisitor`)
• switch identify_components to use the StreamBasedExpressionVisitor

This allows us to (finally) deprecate the SimpleExpressionVisitor and remove it from the documentation

Changes proposed in this PR:

• (see above)

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

1. I agree my contributions are submitted under the BSD license.
2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

[mrmundt]

Tiniest of changes

[Robbybp]

Performance comparison on my motivating example for the previous identify_variables rewrite:

Main

Identifier                                       ncalls   cumtime   percall      %
----------------------------------------------------------------------------------
root                                                  1     2.905     2.905  100.0
     -----------------------------------------------------------------------------
     full model post-solve                            1     0.484     0.484   16.7
     solve-scc                                        1     2.421     2.421   83.3
                          --------------------------------------------------------
                          igraph                      1     0.359     0.359   14.8
                          scc-subsolver             546     1.610     0.003   66.5
                          vars-from-components      546     0.187     0.000    7.7
                          other                     n/a     0.264       n/a   10.9
                          ========================================================
     other                                          n/a     0.000       n/a    0.0
     =============================================================================
==================================================================================

This branch

Identifier                                       ncalls   cumtime   percall      %
----------------------------------------------------------------------------------
root                                                  1     2.960     2.960  100.0
     -----------------------------------------------------------------------------
     full model post-solve                            1     0.482     0.482   16.3
     solve-scc                                        1     2.478     2.478   83.7
                          --------------------------------------------------------
                          igraph                      1     0.358     0.358   14.4
                          scc-subsolver             546     1.536     0.003   62.0
                          vars-from-components      546     0.326     0.001   13.1
                          other                     n/a     0.259       n/a   10.4
                          ========================================================
     other                                          n/a     0.000       n/a    0.0
     =============================================================================
==================================================================================

Less than a factor of 2 overhead (see the vars-from-components category) and still much better than the 2s this was taking before exploiting named expressions. Thanks for fixing this.

[Robbybp]

Is there any reason we need to store e.expr in addition to e?

[Robbybp]

I see that we need to store the immutable expression object in case e changes what expression it contains.

[Robbybp]

To check my understanding: This means that if named_expression_cache was not provided in __init__, we won't exploit repeated named expressions within this expression?

[jsiirola]

Correct. My assumption is that the same named expression rarely appears twice in a single expression. So, if you don't provide a cache, then there isn't a big win for defining a cache -- there probably won't be a cache hit (and there is the overhead of creating the cache and throwing it away).

[Robbybp]

That's probably a good assumption in general, but I know that for some IDAES models (at least the autothermal reformer), there is a significant benefit to exploiting named expressions within a single constraint. (Of course the user can always do this by explicitly passing the named expression cache.)

[Robbybp]

Why use a global visitor instead of a new one every time identify_variables is called? Is there a significant overhead to __init__?

[jsiirola]

There is a bit of overhead in object creation and disposal. In other cases, I have seen performance benefits by keeping the visitor around between calls. I did not profile the impact here, so maybe this is a red herring?

[Robbybp]

I have seen this in AMPLRepnVisitor, so I'm not too surprised, but it hasn't shown up in any of my profiles.

[jsiirola]

@Robbybp: I am surprised that the overhead went up. can you share your test with me (off-line is fine)? I wonder if we are measuring something else (like the GC)?

[Robbybp]

@jsiirola Run this script: https://github.com/Robbybp/surrogate-vs-implicit/blob/main/svi/auto_thermal_reformer/fullspace_flowsheet.py

I insert the timing calls into incidence_analysis.scc_solver and util.subsystems manually when I profile this, so this won't give you the detailed profile, but you should still see the runtime jump.

[blnicho]

We are waiting to merge this until we double check the performance degradation @Robbybp noted

[Robbybp]

FWIW, the evidence I presented for a performance degradation is pretty flimsy (~0.15s diff), so I wouldn't be opposed to just merging this as-is, given the bug fixes.

[jsiirola]

@Robbybp I finally got a chance to look at this, and there was an issue with the implementation. The original approach to validating the named expression cache involved storing a list of all expressions seen in the current expression or any sub-expression. Because of how interconnected named expressions are in IDAES models, this would occasionally get to be a very long list (>1k!). The implementation has been updated to store this as a dict, which prunes redundant instances of the same subexpression (so the list stays under 20 long).

This PR's implementation is now somewhere between equivalent to the old implementation to possibly slightly faster (the noise in timing in Python is now more than the difference between the two implementations) for your case.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.62%. Comparing base (61e28af) to head (db9c375).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3436      +/-   ##
==========================================
+ Coverage   88.61%   88.62%   +0.01%     
==========================================
  Files         880      880              
  Lines      100639   100646       +7     
==========================================
+ Hits        89181    89198      +17     
+ Misses      11458    11448      -10     

Flag	Coverage Δ
linux	86.20% <100.00%> (+<0.01%)	⬆️
osx	76.20% <100.00%> (+0.01%)	⬆️
other	86.72% <100.00%> (+<0.01%)	⬆️
win	84.67% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Robbybp]

The recent commits look good to me, thanks for looking into this.

[blnicho]

I found a minor issue in the docs that needs to be fixed but otherwise this looks fine