Skip to content

Feature/result dict wrapper #1195

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

Draft
wants to merge 30 commits into
base: dev
Choose a base branch
from
Draft

Feature/result dict wrapper #1195

wants to merge 30 commits into from

Conversation

p-snft
Copy link
Member

@p-snft p-snft commented May 27, 2025

This adds a drop-in replacement for processing.results that uses the new solph.Results class. It means to provide backwards compatibility while radically simplifying the processing code.

p-snft added 2 commits May 26, 2025 18:12
Compatibility is according to documentation and not fully tested.
@p-snft p-snft self-assigned this May 27, 2025
@p-snft p-snft changed the base branch from master to dev May 27, 2025 19:13
p-snft added 3 commits May 27, 2025 21:14
To allow this esp. with detailed Components that feature custom variables,
all data is treated as sequential if not stated otherwise.
@p-snft
Copy link
Member Author

p-snft commented Jun 2, 2025

This one revealed a problem we currently have: Multiple blocks (in particular NonConvexFlowBlock and InvestNonConvexFlowBlock) define the same variable names. The class Results, however, expects every variable name to be present only once.

In fact, we might not want to define the variables multiple times but have one set include the other.

@Bachibouzouk
Copy link
Contributor

NonConvexFlowBlock and InvestNonConvexFlowBlock) define the same variable name

Do you mean status and status_nominal ?

@p-snft
Copy link
Member Author

p-snft commented Jun 2, 2025

For example, we have

self.status = Var(self.NONCONVEX_FLOWS, m.TIMESTEPS, within=Binary)

in NonConvexFlowBlock and
self.status = Var(
self.INVEST_NON_CONVEX_FLOWS, m.TIMESTEPS, within=Binary
)
in InvestNonConvexFlowBlock (which is a child class of NonConvexFlowBlock).

@p-snft
Copy link
Member Author

p-snft commented Jun 4, 2025

Turns out that I misinterpreted as one Block as the child class of the other. We have the following situation:

  • We can have the same variable name in multiple blocks (Pyomo allows this).
  • We have the convention that one Entity (Node or Flow) is only covered by one Block.
  • The status of a Flow is defined either in NonConvexFlowBlock or in InvestNonConvexFlowBlock. As the function _create(self, group=None) is not called by the Constructor, the variable exists only once per Flow.

Now, there are two possible ways:

  1. Make INVEST_NON_CONVEX_FLOWS a subset of NONCONVEX_FLOWS, so that only one status variable is present. In that case, we need to access variables outside the Block (either global for the model or one block to the other.)
  2. Change the results object to be able to handle variables with the same name as long as the indexes are disjunct. In that case, NON_CONVEX_FLOWS should rename to someone that reflects the fact that they do not include those with investment. Maybe FIXED_CAPACITY_NONCONVEX_FLOWS.

In my opinion, way 2 is more versatile and easier to implement. Independent from this, I'd suggest to separate the Blocks so one is no longer the child of the other. (We can of course still reuse functions.)

p-snft added 7 commits June 5, 2025 10:05
This is to make clear that NonConvexFlowBlock and InvestNonConvexFlowBlock
are responsible for disjunct sets of Flows. Historically, NONCONVEX_FLOWS
were not allowed to have an investment, but with INVEST_NON_CONVEX_FLOWS
around, the name is now confusing. Thus, the set name NONCONVEX_FLOWS is
replaced by FIXED_CAPACITY_NONCONVEX_FLOWS, which clearly complements
INVEST_NON_CONVEX_FLOWS.

Additionally, InvestNonConvexFlowBlock is no longer inheriting from
NonConvexFlowBlock. Mistakenly assuming that NONCONVEX_FLOWS could have an
investemnt, one might have expected that some things were done for all
of these in NonConvexFlowBlock.__init__(). This, however was never the
case. Both classes do not share data. To still avoid code duplicaton,
re-usable methods in NonConvexFlowBlock are now staticmethods.
Historically, NONCONVEX_FLOWS were not allowed to have an investment,
but with INVEST_NON_CONVEX_FLOWS around, the name is now confusing.
Thus, the set name NONCONVEX_FLOWS is replaced by
FIXED_CAPACITY_NONCONVEX_FLOWS, which clearly complements
INVEST_NON_CONVEX_FLOWS.

However, there are sets in the flows with names that are present in both,
NonConvecFlowBlock and InvestNonConvecFlowBlock (e.g. STARTUPFLOWS).
Thus, it still makes sense to derive one class from the other.
The constrained ignored flows with a capacity investment.
As the Blocks are created uninitialised, they might not have the sets
created. This then causes a crash. This works around this. (However,
it was cleaner to create the sets on initialisation, if possible.)
@Bachibouzouk
Copy link
Contributor

In my opinion, way 2 is more versatile and easier to implement. Independent from this, I'd suggest to separate the Blocks so one is no longer the child of the other. (We can of course still reuse functions.)

I would also vote for 2

p-snft and others added 5 commits June 5, 2025 12:02
NaN values should no longer cause errors.
This changes the results object to be able to handle variables with the
same name (present in different Blocks) as long as the indexes
are disjunct.

Analysis of the situation:
* We have the convention that one Entity (Node or Flow)
  is only covered by one Block.
* The status of a Flow is defined either in NonConvexFlowBlock
  or in InvestNonConvexFlowBlock. As the function
  _create(self, group=None) is not called by the Constructor,
  the variable exists only once per Flow.
@p-snft
Copy link
Member Author

p-snft commented Jun 5, 2025

I just implemented the approach in #1202. (It's a separate PR as it fixes a problem also present even if we do not merge this one.)

p-snft and others added 6 commits June 5, 2025 13:46
This changes the results object to be able to handle variables with the
same name (present in different Blocks) as long as the indexes
are disjunct.

Analysis of the situation:
* We have the convention that one Entity (Node or Flow)
  is only covered by one Block.
* The status of a Flow is defined either in NonConvexFlowBlock
  or in InvestNonConvexFlowBlock. As the function
  _create(self, group=None) is not called by the Constructor,
  the variable exists only once per Flow.
@p-snft
Copy link
Member Author

p-snft commented Jun 5, 2025

What is now missing is just the support for experimental features (TSAM and multi-periods).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants