-
Notifications
You must be signed in to change notification settings - Fork 228
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.
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
Add reference to constructing class in ProcessBlock #1414
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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 |
@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 I was hesitant to put something like this in myself, but it's a generalization of the proceeding lines generating |
@jsiirola is aware of this and exploring possible solutions. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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.
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.
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
orMyBlockData
.Changes proposed in this PR:
process_block_class
to the_ScalarProcessBlockMeta
and_IndexedProcessBlockMeta
metaclassesLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: