Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add reference to constructing class in ProcessBlock #1414

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Add reference to constructing class in ProcessBlock #1414

merged 3 commits into from
Aug 22, 2024

Conversation

bknueven
Copy link
Contributor

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:

  • Add function process_block_class to the _ScalarProcessBlockMeta and _IndexedProcessBlockMeta metaclasses
  • Add tests for the new metho

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
Copy link

codecov-commenter commented May 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files 
@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewlee94 andrewlee94 requested a review from jsiirola May 21, 2024 12:30
@andrewlee94
Copy link
Member

andrewlee94 commented May 21, 2024 •
edited
Loading

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
Copy link
Contributor Author

@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.

bknueven added a commit to bknueven/watertap that referenced this pull request May 21, 2024
@bknueven
add aggregate -- needs IDAES/idaes-pse#1414 (for now)
00d9774
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label May 23, 2024
@ksbeattie
Copy link
Member

@jsiirola is aware of this and exploring possible solutions.

@ksbeattie
Copy link
Member

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

@ksbeattie ksbeattie added Priority:High High Priority Issue or PR and removed Priority:Normal Normal Priority Issue or PR labels Jul 11, 2024
@ksbeattie
Copy link
Member

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

@ksbeattie
Copy link
Member

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.

andrewlee94
andrewlee94 approved these changes Aug 15, 2024
View reviewed changes
Copy link
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

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

Having spoken to @bknueven about the exact use case they want, I see why he is doing it this way and it does make sense. Unless @jsiirola can find the time and come up with a more elegant solution, I suggest we find someone else to give a review and merge this.

jsiirola
jsiirola approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@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.

This feels like a bit of a hack, but I am having a hard time articulating why it feels like a hack. I think this is a special case of a more general need that should probably be added upstream in Pyomo. That said, there is nothing fundamentally wrong with this implementation - apart from expanding an API that we will have to support for a long time.

@ksbeattie ksbeattie enabled auto-merge (squash) August 22, 2024 20:32
@ksbeattie ksbeattie merged commit a1be331 into IDAES:main Aug 22, 2024
37 checks passed
lbianchi-lbl pushed a commit to watertap-org/watertap that referenced this pull request Sep 27, 2024
@bknueven @kurbansitterley
adding LCOW per unit and per flow (#1398)
31100db 
* adding LCOW per unit and per flow

* by unit model name, not costing block name

* breakdown by individual flow

* Revert "breakdown by individual flow"

This reverts commit 7038557.

* Implement @adam-a-a's suggestions

* break out opex / capex

* adding aggregate

* add aggregate -- needs IDAES/idaes-pse#1414 (for now)

* better aggregation of variable unit costs

* better test

* add flow component breakdowns

* patch for expressions

* minor cleanup; cover a few more edge cases in unit model discovery for flows

* address commments from code review

* fix units error for multiplier

* add units check; add check for SECI

---------

Co-authored-by: Kurban Sitterley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewlee94 andrewlee94 andrewlee94 approved these changes

@jsiirola jsiirola jsiirola approved these changes

@dallan-keylogic dallan-keylogic Awaiting requested review from dallan-keylogic dallan-keylogic is a code owner

@lbianchi-lbl lbianchi-lbl Awaiting requested review from lbianchi-lbl lbianchi-lbl is a code owner

Assignees

@bknueven bknueven

Labels
Priority:High High Priority Issue or PR WaterTAP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bknueven @codecov-commenter @andrewlee94 @ksbeattie @jsiirola