IDAES / idaes-pse

The IDAES Process Systems Engineering Framework
https://idaes-pse.readthedocs.io/
Other
217 stars 235 forks source link

Function to get state block from port #480

Closed eslickj closed 3 years ago

eslickj commented 3 years ago

We have a function to get a state block from a port in the tables utility module. Right now it's private, but it seems like something we may want to make public. It's probably just a matter of removing the underscore and making sure there are tests and documentation.

dallan-keylogic commented 3 years ago

I've tried to use this method on a few ports in some of the example flowsheets, and have encountered unexpected behavior. For the Flash unit, outlet blocks do not have associated state blocks: flash_behavior Meanwhile, in an example with a TrayColumn, there is a scenario where one port extends another and the higher level port is connected to the Arc, while the lower level port has the associated property package: distillation_behavior

What should the intended behavior be in these situations?

The solution might not be exposing this method, but rather creating other methods to allow users to get the information in the port's associated state block.

eslickj commented 3 years ago

Yeah, I have some cases where I have ports that don't have state blocks associated. My feeling is that there is nothing to do about it, but raise a clear exception.

dallan-keylogic commented 3 years ago

For my application, I need to be able to associate material flow streams with the state blocks that determine their properties. If you have ports connecting, for example, two temperatures in different heat exchanger blocks, I wouldn't care about not being able to find a state block. But I would need to get a state block to get physical properties for the stream originating from this flash port. Otherwise, it's useless to me, because I'll need to find a workaround anyway.

Is there another use case for exposing this function besides console debugging?

eslickj commented 3 years ago

My thinking was for convenience it would be nice. A lot of times I just use ports as shortcuts to state variables. It's a lot easier especially when you are not super familiar with the model structure. I guess not being able to count on this being able to get to a state block, may be a show stopper though.

eslickj commented 3 years ago

I guess this thing even in the private use has holes in it, for generating stream tables and stuff. Maybe for ports that don't have state blocks we should have a sort-of dummy state block. That still lets you make the proper connections.

andrewlee94 commented 3 years ago

For the case of an "extended" Port, you can get the original Port from the _extends attribute on the Port.

eslickj commented 3 years ago

There are things like surrogates and bad people like me that wrote a unit model with no state blocks.

andrewlee94 commented 3 years ago

In the case of surrogates at least, you can't guarantee conservation of anything anyway so @dallan-keylogic doesn't need to worry about that one.

eslickj commented 3 years ago

Well that leaves my laziness to deal with. The mass balance and energy balance hold for the unit (the one with no state blocks). You should be able to use the ports to do an overall mass balance, but the enthalpy number may not consistent with with other property packages. What's needed? Like a dummy state block with the get_whatever_massbalance_terms() methods?

I also have a port on the same unit that multiplies the flow. This unit may have millions in parallel, to to deal with that I just added a port that is the same at the single unit port and multiplies it by the number of cells. We could add a dummy state block there too. Of course selecting the wrong port for the mass balance testing tool would be bad.

Maybe my fondness for short cuts is a problem, but I think if we could guarantee either a real state block or a dummy state block that provided the required stuff, we could make this work. Maybe that could be a model construction check that could warn you these ports don't have state blocks, so some model utilities may not work.

dallan-keylogic commented 3 years ago

For the case of an "extended" Port, you can get the original Port from the _extends attribute on the Port.

I'm having trouble imagining situations where having the original Port continue to exist is a good thing. It seems like both references should point towards the same Port. In the Flash unit, we have an input port, two output ports that extend those from the separator unit, then the separator has its own input port that appears to be an orphan. Do we want to train users to just not look for ports except at the highest level of the unit block? The TrayColumn breaks this convention, but I'll complain about that in #96 .

In the case of surrogates at least, you can't guarantee conservation of anything anyway so @dallan-keylogic doesn't need to worry about that one.

Also the user would choose which ports to register as inputs and outputs. I'm comfortable blaming the user if they try to draw the boundaries around their own surrogate model without property blocks. Cases like the flash where you'd think there would be property blocks are more concerning.

Well that leaves my laziness to deal with. The mass balance and energy balance hold for the unit (the one with no state blocks). You should be able to use the ports to do an overall mass balance, but the enthalpy number may not consistent with with other property packages. What's needed? Like a dummy state block with the get_whatever_massbalance_terms() methods?

Yeah, just some methods like get_material_flow_terms(p,j) and get_enthalpy_flow_terms(p). My intention was to make this available as a debugging tool for the user's benefit. If you want to keep some idiosyncratic unit model that doesn't have those methods, that's fine. I assume you translate back to property blocks somewhere else.

Ideally, I would want people to add enthalpy of formation terms into the way they define their properties. Then you could expect energy balances to hold around translator blocks as well. When you have things like translator blocks in a flowsheet, it's always a (small) possibility that IPOPT decides to increase throughput by exploiting arbitrage between translator blocks to generate mass and energy.

andrewlee94 commented 3 years ago

@dallan-keylogic There is a very good reason for having both Ports in the distillation column - the column is made up of a number of sub-models which are internally connected by Arc based in the internal Ports - you just don't see those Arcs from the outside.

andrewlee94 commented 3 years ago

I found the problem with the existing method to find state blocks from Ports - it depends on a _state_block attribute which is created by the IDAES add_port methods in UnitModel - i.e. it is not relying on underlying Pyomo methods. This means that if a Port is created directly using the Pyomo component then this attribute is missing. As there are many reason why we might want to use the base Pyomo component, this strikes me as extremely fragile, which we are seeing here.

An alternative approach would be to assume that all elements of the Port come from the same Pyomo Block (generally a State Block), at which point you could do something like:

for e in port.values():
    return e.parent_block()

Not particularly elegant, but it should work. However, this does depend on the assumption that the first element of the Port that we collect will come from the same Block as everything else. At least for IDAES models, this should hold in most cases, and the cases where it doesn't hold probably indicate that the developer should reconsider how they are doing things.

Thus, I think that the current method needs to be redesigned to be less fragile, as this will probably come back to bite the UI team at some point as well.

@jsiirola Do you have any suggestions on better ways to do this?

dallan-keylogic commented 3 years ago

The specific problem with the TrayColumn unit is that it generates a feed port that extends a feed port that is part of a tray. The tray's port is given a _state_block attribute by add_port, but this attribute isn't copied over when the port is extended. Adding a function to create private attributes for a class that doesn't support them natively seems to me like playing with fire. Creating either a child class of Port or a superclass StreamInlet and giving them methods to get all the stuff that I want and the UI team wants seems like a solution, but I am not learned in the art of OOP. Obviously, this would be a lot of work, but if such a change is warranted, better to do it earlier than later.

The other half of the problem is that the methods using this function depend on looking at the destination end of an arc for material properties. It seems to me like it's reasonable to ask the outlet port of a solved unit about its stream properties without depending on whether an inlet port for a successor unit even exists, much less whether that unit's been solved. The hacky solution is to make a temporary arc to a temporary Product block, solve that unit, and then get properties from there. I'm not sure whether removing them with del_component() removes all the other Pyomo objects created by such a procedure.

andrewlee94 commented 3 years ago

@dallan-keylogic For your first point, I think you are looking at the problem the wrong way - it is not a problem with a the TrayColumn, but a problem with the way the method to get state blocks from Ports was designed. The current method is a hack that only works if you use specific IDAES methods, and thus does not consider extending Ports or cases where a developer might want to add a Port directly. Thus, the problem is the hack, not the use of extended Ports in the TrayColumn.

The second part is part of the separate but related issue of Ports without state blocks. This is a very different, and much hard, problem to deal with so should be considered separately.

I would suggest focusing on solving the first problem before trying to address the second.

jsiirola commented 3 years ago

@andrewlee94, It's slightly more complicated, but not by much. I would try something like:

def get_state_block_from_port(port):
    states = list(v.parent_block() for v in port.iter_vars())
    if all(states[0] is s for s in states):
        return state[0]
    return None

(the trick is iter_vars, which expands indexed things in Ports -- and is critical to get the actual VarData / ExpressionData and not the Reference from the Unit model)