Pyomo Network #583
Pyomo Network #583
Conversation
Codecov Report
@@ Coverage Diff @@
## master #583 +/- ##
==========================================
+ Coverage 65.97% 66.24% +0.27%
==========================================
Files 379 383 +4
Lines 60341 61011 +670
==========================================
+ Hits 39809 40418 +609
- Misses 20532 20593 +61
Continue to review full report at Codecov.
|
|
In addition to the whole |
|
I personally think that the reclassification approach makes sense. You're going from the higher-level modeling space of having Connections to the lower-level space of groups of constraints. Is there still a need for the higher level information after transformation? |
|
@qtothec you would be unable to search the model for the |
|
As I said, one of the options is to change the model walker to find everything that is a subclass of Block. An automatic way to do this would be with a helper function that finds all defined subclasses of a class, like so: def all_subclasses(cls):
all_subs = set()
for sub in cls.__subclasses__():
all_subs.add(sub)
all_subs.update(all_subclasses(sub))
return all_subsFor example, after importing pyomo.environ, for Block this gets: ComplementarityList, IndexedDisjunct, IndexedComplementarity, IndexedBlock, SimpleDisjunct, IndexedPiecewise, SimpleComplementarity, Disjunct, SimplePiecewise, Complementarity, IndexedConnection, SubModel, SimpleBlock, Piecewise, AbstractModel, ConcreteModel, Model, Connection, SimpleConnection. Meanwhile, just I don't know if we want to make it look for every defined subclass or not. Alternatively, we can just explicitly add |
|
This should now be fully fleshed out and ready for review. A major change from when I first opened this PR is that connections are no longer blocks, they are base level (active) components, and when they are expanded they create blocks on which the constraints are added. One thing I'm still unsure of and would like some opinions on is the naming conventions for the expanded constraints. It currently works like this, for a connection named The block where everything goes is called On the block, there are scalar constraints for scalar connector vars. They are named like For indexed connector vars, there are At the bottom there's an example of what the model looks like after expanding. I'm not 100% sure this is the best naming convention, especially since you have to use the Also @qtothec and @jsiirola would you mind reviewing? m = ConcreteModel()
m.flow = Var(['a', 'b', 'c'])
m.temperature = Var()
m.pressure = Var()
m.con = Connector()
m.con.add(m.flow)
m.con.add(m.temperature)
m.con.add(m.pressure)
m.econ = Connector()
m.connect = Connection(connectors=(m.con, m.econ))
ConnectionExpander().apply(instance=m)
for i in m.component_data_objects(): print(i) |
|
You should be aware of this: #525 |
|
A couple comments and opinions:
Here are some naming suggestions: Also, you should know about this utility function for guaranteeing unique component names which might be useful for ensuring a unique name for the pyomo/pyomo/common/modeling.py Line 23 in e34a057 |
|
@blnicho thanks for the comments.
And thanks for the naming tips. |
|
A comment on the minor change I just made, I realized you don't need the bal constraint (sum of in == sum of out) since you already get this from the insum and outsum constraints, and actually having that extra constraint violates LICQ. Also I forgot to change the test to reflect this so I'm about to push another commit for that... |
|
@gseastream: while I can see advantages of propagating domains to either auto or extensive variables (particularly for preprocessing transformations), I believe that I agree with @qtothec's implication that it doesn't matter if the variable domains match -- as long as the auto variable's domain is not more restrictive than the "other" variable. |
pyomo/core/base/connection.py
Outdated
| logger = logging.getLogger('pyomo.core') | ||
|
|
||
|
|
||
| class _ConnectionData(ActiveComponentData): |
There was a problem hiding this comment.
Following Pyomo convention, _*Data classes should be slotized.
| # ___________________________________________________________________________ | ||
|
|
||
| from pyomo.common.plugin import PluginGlobals | ||
| PluginGlobals.add_env("pyomo") |
There was a problem hiding this comment.
Pyomo generally avoids importing/registering plugins in the global environment. That said, I am not always sure why that has become the standard. @whart222?
Upon further review, it doesn't look like you are registering plugins. Can the Globals push be omitted?
There was a problem hiding this comment.
I was just copying the __init__.py files of other packages I saw. Other packages (like gdp, dae, pysp, repn) do this exact same thing. I have no comment on or idea of what it does. As for registering plugins, I'm not sure what exactly that means but I do make calls to alias and register_component.
pyomo/network/arc.py
Outdated
| "argument 'ports' must be list or tuple " | ||
| "containing exactly 2 Ports.") | ||
| for c in ports: | ||
| if type(c) not in port_types: |
There was a problem hiding this comment.
I would prefer checking the ctype and not the actual class type. That is: if c.type() is not Port.
There was a problem hiding this comment.
I only did it this way in case someone passed non-component types (without a .type() method), but I can put it in a try-except to check the ctype.
pyomo/network/arc.py
Outdated
| "containing exactly 2 Ports.") | ||
| for c in ports: | ||
| if type(c) not in port_types: | ||
| if type(c) is IndexedPort: |
There was a problem hiding this comment.
This should test if c.is_indexed():
pyomo/network/arc.py
Outdated
| "must specify both 'source' and 'destination' " | ||
| "for directed Arc.") | ||
| if type(source) not in port_types: | ||
| if type(source) is IndexedPort: |
There was a problem hiding this comment.
These tests should be updated as mentioned above.
| name == "splitfrac"): | ||
| raise ValueError( | ||
| "Extensive variable '%s' on Port '%s' may not end " | ||
| "with '_split' or '_equality'" % (name, self.name)) |
There was a problem hiding this comment.
You can still end up with name collisions. Consider the following (insanity):
m.p = Port()
m.p.add(m.x, 'foo_equality') # rule=Port.Equality
m.p.add(m.foo, rule=Port.Extensive)
There was a problem hiding this comment.
Name collisions still won't occur in this case. Basically, every constraint that ends up on the expanded block ends in either _equality or _split. For adding new expanded/extensive variables to the expanded blocks, those new variables have the exact same name as the port member. So if we make sure they don't end in _equality or _split (and that they're not named splitfrac), we can ensure they will not conflict with any other names on the extensive block. Furthermore, as for your insane example, the equality constraint generated for your foo_equality member will be named foo_eqaulity_equality, and the expanded block will then contain a constraint of this name as well as (depending on if the connectivity requires it) a variable named foo and another named splitfrac and a constraint named foo_split.
pyomo/network/port.py
Outdated
|
|
||
| def _iter_vars(self): | ||
| for var in itervalues(self.vars): | ||
| if not hasattr(var, 'is_indexed') or not var.is_indexed(): |
There was a problem hiding this comment.
Why do er need the hasattr? Is this a Kernel thing?
There was a problem hiding this comment.
According to this git blame for connectors, you introduced using hasattr 2 years ago with the following commit message:
Removing requirement that Var, Connector support is_indexed()
This fixes what was almost certainly a bug in the original expression
API, where we wanted to be able to tell a user when they attempted to
use an indexed Var where they probably meant to use one of the contained
_VarData objects. To give a good error, we checkedis_indexed()-
which suddenly made Var containers support the VarData API. This commit
removes that requirement.
Is the check no longer necessary?
pyomo/network/port.py
Outdated
| ((k, v) for k, v in iteritems(self._data)), | ||
| ("Name", "Value"), _line_generator) | ||
|
|
||
| def Equality(port, name, index_set): |
There was a problem hiding this comment.
Should this be declared a @classmethod?
There was a problem hiding this comment.
I just read up on the difference between class and static methods here, and it looks like I would prefer static methods. Class methods require that the first argument be a reference to a class object, but that would be unnecessary here since I don't need a handle on Port as an argument. Not only that, it would change the api of passable rule functions to have to be something that takes a class as its first argument which is weird because it's unnecessary, and then that affects users who may want to define their own custom expansion function and use that as the rule.
That being said, I do think it would look cleaner to use the decorators instead of the staticmethod function calls below, so I'll probably switch to using that.
EDIT: I read further into it and I see that while classmethods have the class as the first argument, it is implicit and does not need to be passed when calling the function. Still, I don't feel like I need the handle on Port as an argument, and as a staticfunction it will be clearer that this is just a plain old ordinary function that happens to be located inside Port.
pyomo/network/port.py
Outdated
| for arc in port.arcs(active=True): | ||
| Port._add_equality_constraint(arc, name, index_set) | ||
|
|
||
| def Extensive(port, name, index_set, write_var_sum=True): |
There was a problem hiding this comment.
Should this be declared a @classmethod?
| """ | ||
| port_parent = port.parent_block() | ||
| out_vars = Port._Split(port, name, index_set, write_var_sum) | ||
| in_vars = Port._Combine(port, name, index_set) |
There was a problem hiding this comment.
Any reason why you don't call these methods as port._Split(name, index_set, write_var_sum)? That way they don't need to be declared as static methods below.
There was a problem hiding this comment.
I don't consider them methods of the instance, but rather transformation functions that can be called on a port, much in the same vein as Extensive. I wouldn't want to confuse instance methods for transformation functions that happen to be stored in the class definition. And theoretically, they can be used as the rule instead of Extensive if for whatever reason the user only wants to split things and never mix, so they follow the same api as Extensive.
Summary/Motivation:
This originated out of the desire to have IDAES Streams inherit off of a Pyomo component in order to be able to genericize the sequential modular simulator I'm working on. However, it's also a useful Pyomo component since it provides a simple API for equating everything in two Connectors, and expanding Connections is less expensive than the current
ConnectorExpandersince it is able to search the model for the specific Connection ctype.Basically a Connection is a component on which you can define either a source/destination pair for a directed Connection or simply pass a list/tuple of two Connectors for an undirected Connection. After expanding, simple equality constraints are added onto a new block and the connection is deactivated.
Except it's all called ports and arcs now and it's in a new package and it doesn't have to be just an equality relationship.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: