Skip to content

Reworking unhashable indexing#467

Merged
whart222 merged 9 commits intomasterfrom
expr_indexed
Jun 12, 2018
Merged

Reworking unhashable indexing#467
whart222 merged 9 commits intomasterfrom
expr_indexed

Conversation

@whart222
Copy link
Copy Markdown
Member

@whart222 whart222 commented May 5, 2018

Fixes N/A.

Summary/Motivation:

Eliminate duplicate walking of the expression tree when processing unhashable index values.

Changes proposed in this PR:

This PR creates an extension to evaluate_expression() with the 'constant' argument. When constant=True, this raises exceptions that reflect the key error messages that we want to return: (a) variables and (b) mutable parameters.

This change doesn't directly impact Pyomo performance in most cases, but the following script illustrates a case where the new code is twice as fast as the master branch:

from pyomo.environ import *
import time

m = ConcreteModel()
m.A = RangeSet(1000000)
m.p = Param(m.A, initialize=0, mutable=True)
m.v = Var(m.A, initialize=1)

i = sum(m.p[i] for i in m.A)

start = time.clock()
try:
    print(m.v[i])
except:
    pass
print(time.clock()-start)

Legal Acknowledgement

By contributing to this software project, I 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.

Using evaluate_expression() with constant=True.  This raises
exceptions that reflect the key error messages that we want to return.
Altogether, this avoids evaluating an expression tree twice.
@codecov-io
Copy link
Copy Markdown

codecov-io commented May 5, 2018

Codecov Report

Merging #467 into master will increase coverage by 0.01%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #467      +/-   ##
=========================================
+ Coverage   64.99%     65%   +0.01%     
=========================================
  Files         357     357              
  Lines       59681   59699      +18     
=========================================
+ Hits        38787   38806      +19     
+ Misses      20894   20893       -1
Impacted Files Coverage Δ
pyomo/core/base/indexed_component.py 89% <100%> (+0.31%) ⬆️
pyomo/core/expr/expr_pyomo5.py 98.42% <96.29%> (-0.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 5685752...931ff39. Read the comment docs.

@whart222 whart222 requested a review from jsiirola May 18, 2018 04:07
Copy link
Copy Markdown
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a couple passes to figure out what you are doing here and why (I can't imagine how you came across this).

Overall the change makes sense. All of my comments are Editorial, but there are enough that i would like to see a quick cleanup commit before we merge. I am happy to do that if you need.



if node.is_variable_type():
if node.fixed:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this using an attribute instead of the is_fixed method?

# Disable all logging while evaluating the expression
#
#logging.disable(logging.CRITICAL)
try:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment and the associated commented code should be removed.

value() function.""" % ( self.name, i ))

except ValueError:
#
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This except clause doesn't do anything and should be removed.

raise

#finally:
# logging.disable(logging.NOTSET)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code should be removed.

CHANGELOG.txt Outdated
- Adding TerminationCondition and SolverStatus to pyomo.environ (#429)
- Resolved problems with ExternalFunctions with fixed arguments (#442)
- Adding the Pyomo5 expression system, which supports PyPy (#272)
- Reworking unhashable indexing (#467)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is editorial, but we didn't rework indexing. Could you make it "Efficiency improvements when processing unhashable component indices"?

return False, None


def evaluate_expression(exp, exception=True, constant=False):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring needs "constant" added to it.

@whart222
Copy link
Copy Markdown
Member Author

@jsiirola Sure. Go ahead and make your changes. I'd rather work collaboratively on PRs.

whart222 and others added 6 commits June 7, 2018 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants