IDAES / idaes-pse

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

Add reference to constructing class in ProcessBlock #1414

Closed bknueven closed 1 month ago

bknueven commented 4 months ago

Fixes N/A

Summary/Motivation:

As best as I can tell, I cannot easily get the constructing class back from an instance of a process block. E.g.

from pyomo.environ import ConcreteModel, Var, value
from pyomo.common.config import ConfigValue
from idaes.core import ProcessBlockData, declare_process_block_class

@declare_process_block_class("MyBlock")
class MyBlockData(ProcessBlockData):
    CONFIG = ProcessBlockData.CONFIG()
    CONFIG.declare("xinit", ConfigValue(default=1001, domain=float))
    CONFIG.declare("yinit", ConfigValue(default=1002, domain=float))

    def build(self):
        super(MyBlockData, self).build()
        self.x = Var(initialize=self.config.xinit)
        self.y = Var(initialize=self.config.yinit)

m = ConcreteModel()
m.b = MyBlock()

# how to get a link back to `MyBlock` from `m.b`?
# m.b.__class__ is <class 'idaes.core.base.process_block._ScalarMyBlock'>

I have a use case in WaterTAP which would aggregate some results by unit model type. Ideally we would index by the creating class, e.g., MyBlock in this example, but it does not seem to be possible to get this class back from an instance of _ScalarMyBlock or MyBlockData.

Changes proposed in this PR:

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 license terms described in the LICENSE.txt file at the top level of this directory.
  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.
codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.36%. Comparing base (8bf70ee) to head (17490c0). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1414 +/- ## ========================================== - Coverage 76.36% 76.36% -0.01% ========================================== Files 394 394 Lines 65128 65130 +2 Branches 14427 14429 +2 ========================================== - Hits 49735 49734 -1 - Misses 12831 12835 +4 + Partials 2562 2561 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

andrewlee94 commented 4 months ago

At first glance, I cannot see any major issue with this proposal, beyond having another attribute attached to every model. However, for some reason I am hesitant to do this, although I am not really sure why.

A possible alternative is to look at this the other way - can you get the BlockData class from the Block? If so, you should be able to write a function that takes a Block (or BlockData), gets the associated BlockData class, and then aggregate all instances of that BlockData. I would argue this would be the better way to do it, as the BlockDatas are the concrete classes developers have control over (as opposed to the metaclasses generated by black magic).

bknueven commented 4 months ago

@andrewlee94 the thing I am trying to design is related to costing. In this PR in WaterTAP, I already have code which calculates the contribution of each specific unit model to a particular cost metric.

For certain processes, the same unit model may appear multiple times, and it would be nice to aggregate the costs by the unit model's type, such that the index into each item could take the class itself or the name of the class. This would be a user-facing capability, not a developer-facing capability. I could infer the name of the user-facing class from _ComponentDataClass, but seemed uglier than this solution.

I was hesitant to put something like this in myself, but it's a generalization of the proceeding lines generating base_class_module, in that you could get the same information out of the proposed method. I would generally propose we replace the base_class_module method with the new method process_block_class, but obviously that would need a deprecation path.

ksbeattie commented 3 months ago

@jsiirola is aware of this and exploring possible solutions.

ksbeattie commented 2 months ago

@bknueven what is the real urgency on this? Perhaps coordinating directly with @jsiirola will help move this along?

ksbeattie commented 1 month ago

@jsiirola, @bknueven, any news? This is currently not looking good for an Aug release but WaterTAP needs it.

ksbeattie commented 1 month ago

I'm going to move this to the Nov release, since it doesn't appear likely to be done by the end of this month.