Add handling for NumericConstant to _get_linear_collector in canonical_repn

[qtothec]

In the process of trying to work around #167 by wrapping the substituted value in a NumericConstant, I came across #168 and also now the fact that _get_linear_collector() does not support NumericConstant. This fix adds in that support (in addition to the bugfix in #168) by piggybacking off of handling for parameters.

This is motivated by my desire not to declare a bunch of new parameters just to do some substitution of constant values that I do not intend to save and use again.

[jsiirola]

@qtothec: the fix from #168 is of course fine. For the NumericConstant fix, I would prefer that you add an entry to the _linear_collectors map instead of relying on the isinstance checks (the map lookup is far more efficient than the isinstance calls).

As a matter of principle, can you add tests that exercises this? I am a bit bothered by the fact that this slipped through all unit tests... In particular,

• test the collection of linear expressions with NumericConstant objects in them
• test the linear collection of expressions derived from the I* interface classes that do not appear in the "high-speed" lookup map (_linear_collectors). For this, maybe @ghackebeil can assist, as this is code that he added as part of the Kernel refactor. The goal will be to cover the if tree in the except KeyError block.

[qtothec]

@jsiirola: I did add an entry to the _linear_collectors map as well, I thought: ebcc17a#diff-ac36e141575714662fcdaf34e53e2b1bR757

As for the testing, I'm not completely sure how to manufacture these examples. I only found this because my own code broke.

[codecov-io]

Codecov Report

Merging #169 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
+ Coverage   66.17%   66.18%   +0.01%     
==========================================
  Files         442      442              
  Lines       56759    56761       +2     
==========================================
+ Hits        37561    37568       +7     
+ Misses      19198    19193       -5

Impacted Files	Coverage Δ
pyomo/repn/canonical_repn.py	52.88% <100%> (+0.15%)	⬆️
pyomo/pysp/scenariotree/tree_structure.py	84.78% <0%> (+0.23%)	⬆️
pyomo/solvers/plugins/smanager/phpyro.py	88.94% <0%> (+1.05%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6d5716...938ba34. Read the comment docs.

[whart222]

NOTE: This change will be superceded by changes in the expr_dev branch, where the canonical representation has been removed.

[blnicho]

I agree with @jsiirola about the need for more tests here. However, given @whart222's comment, maybe it is best to close this and revisit it after merging #272

[qtothec]

If the changes in #272 fix this, then that's perfectly fine with me. Is #272 stable enough for me to start using it?

On Wed, Jan 3, 2018, 13:10 Bethany Nicholson ***@***.***> wrote: ***@***.**** commented on this pull request. I agree with @jsiirola <https://github.com/jsiirola> about the need for more tests here. However, given @whart222 <https://github.com/whart222>'s comment, maybe it is best to close this and revisit it after merging #272 <#272> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#169 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFowrDG03BAz6cvZgGjYxCGbOK-XcAPWks5tG9BkgaJpZM4N7VYH> .

-- Qi Chen PhD Candidate Center for Advanced Process Decision-Making [email protected]

[jsiirola]

I am merging this as-is: as this will be superseded by #272, I think it is OK that we not explicitly test this (although, it would be good to have a test to ensure that #272 will still work). We shouldn't hold this fix up for #272.

[jsiirola]

Approved.