-
Notifications
You must be signed in to change notification settings - Fork 29
Config validation #684
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
base: main
Are you sure you want to change the base?
Config validation #684
Conversation
…ssume into config_validation
| for market_id, market_config in self.markets.items(): | ||
| market_start = market_config.opening_hours[0] | ||
| market_end = market_config.opening_hours[-1] | ||
|
|
||
| if market_start < self.start or market_end > self.end: | ||
| msg = (f"Market {market_id} violates world schedule. \n" | ||
| f"Market start: {market_start}, end: {market_end}. \n)" | ||
| f"World start: {market_start}, end: {market_end}.)") | ||
| raise ValueError(msg) |
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.
maybe its best to move this to the add_market method so the simulation fails even earlier, when adding an invalid market 🤔
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.
I think you are right, that add_market would be the best place for this part if we organize validation locally. Regarding a possible DataValidation class, to me it sounded more like that the validation is intended to be done centrally for all data.
| unit_operators = list(self.unit_operators.values()) | ||
| for operator in unit_operators: | ||
| for market_id in operator.portfolio_strategies.keys(): | ||
| if market_id not in list(self.markets.keys()): | ||
| msg = (f"Strategies of unit operator {operator} references" | ||
| f"market {market_id} which is not known in world." | ||
| f"Known markets are:\n{list(self.markets.keys())}.") | ||
| raise ValueError(msg) |
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.
see above, this may be moved to the add_unit_operator method
| for market_id in self.markets.keys(): | ||
| if not market_id in referenced_markets: | ||
| msg = f"Added market {market_id}, has no participants." | ||
| warnings.warn(msg) |
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.
I think this part is already ensured by the unit_operator constructor, where a new unit operator references all available markets automatically (i may be wrong though)
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.
Hmm. You are right, there is a similar logic in UnitsOperator.__init__. However (and this is why the corresponding test works): You theoretically can add a market to World after you added all UnitsOperators.
This makes we wonder what the purpose of the respective logic in UnitsOperator.__init__ is.
for market in self.available_markets:
if market.market_id not in self.portfolio_strategies.keys():
self.portfolio_strategies[market.market_id] = (
DirectUnitOperatorStrategy()
)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.
You theoretically can add a market to World after you added all UnitsOperators.
Yes, theoretically - it is currently not supported though. So one first has to create the markets and then add the participants.
Fixing this would require to somehow inject the information of available markets prior to the start of the simulation into all UnitsOperators, which would add another layer of magic/complexity to the simulation.
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.
I am a little confused. Does "Not supported" mean a code like this should raise an Exception?
world = setup_simple_world()
@pytest.fixture
def demand():
return Demand(
id="test_unit",
unit_operator="test_operator",
min_power=0,
max_power=-1000,
technology="demand",
bidding_strategies={},
forecaster=DemandForecaster(index, demand=-100),
)
def test_addition_order(demand):
""" Add participants before market. """
world.add_unit_operator(
"unit_operator",
{"dispatch": "naive_eom"})
world.add_unit_instance(
operator_id="unit_operator",
unit=demand)
opening_dispatch = rr.rrule(rr.HOURLY, dtstart=world.start, until=world.end)
market_products= [MarketProduct(
duration=datetime.timedelta(hours=1),
count=1,
first_delivery=datetime.timedelta(hours=1))]
dispatch_config = MarketConfig(
"dispatch",
opening_hours=opening_dispatch,
market_products=market_products)
world.add_market_operator("market_operator")
world.add_market(
market_operator_id="market_operator",
market_config=dispatch_config)
world.run()| msg = f"Added market {market_id}, has no participants." | ||
| warnings.warn(msg) | ||
|
|
||
| # Real-world integrity. |
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 seems to be perfect for the post_init method of the config, right?
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.
After creation of the market, you do not know if there will be participants.
After creation of a single agent, you do not know if there will be further agents.
So the only place to check if there is any UnitsOperator, which has a strategy for this market configured is right prior to the run execution call of the simulation, by iterating over all units of all UnitsOperators and checking the BiddingStrategies..
|
Thanks @Graf-Wronski! I think the general discussion we have to have is whether this should happen centrally or decentralized. I noticed that @jrasko and I immediately suggested where these have to go in the framework. Two things to consider for this: What is the most natural way if one wants to extend these? Change a validation call in the unit-operator and market, and post_init, or in one validator class? What do you think @Graf-Wronski? |
|
@kim-mskw for now, it is irrelevant for the frontend when the validation takes place, since simulations are executed immeadiatly after creation. |
I am not sure I see the use case of creating a world and then saving it to deploy it somewhere else. When and why would I do that? |
I think with regards to extensibility, a central validation class would be advantageous since already implemented validations are more accesible. Also functions are shorter if validation is outsourced, which I (personally) prefer. The big question mark for central validation however is, where in the pipeline is it triggered. |
This comment was marked as duplicate.
This comment was marked as duplicate.
| for x in dispatch_markets) | ||
| earliest_redispatch_opening = min(x.opening_hours[0] | ||
| for x in redispatch_markets) | ||
|
|
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 logic will not work for markets that open multiple times.
x.opening_hours[0] + x.opening_duration
seems to be a applicable alterantive.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Also, another thing to check. If one wants to use the compley_clearing but does not specify the param_dict it will just give a weird error that the nodes do not match because it initialisses with a default node |
|
I would differentiate between independent misconfigurations and depending misconfigurations. Generally, I would fail as early as possible - see also. I do see good use of a validation method for a pre-run check though.
|
|
@Graf-Wronski this issue is a little bit like a whish list and I guess that some of the comments here should rather be placed as additional entries in #664 This might be overwhelming at first, but remember to always do small steps without trying to solve everything at once :) |
Moved them to the issue :) |
|
So I have been thinking a little a bit about the optimal placement for validation and why it appears a little controversial and tried to summarize our discussion. I think one potential source of controversy is the duality of file-based and script-based world creation. If there was only file-based world creation, I think the best approach would be to load the data and immediately within the DataLoader validate the data (#failearly) before creating a world. For the creation of a world step by step within a script, the case is more complex. There are some validations that appear a little more intuitive when conducted by the respective Now there are these three possible validation locations:
Location 3 is the only one that is suitable as standalone method. Location 1 can only work as a standalone method for file_based scenarios, while location 2 can only work in combination with location 3. If I understand @maurerle correctly, you vote for location 2 where it is possible and location 3 where it is necessary. I personally would go for validation only at location 3 but I could live with 2+3. Are there any other opinions? And does anyone see further need for validation for loaded data (at location 1)? |
Related Issue
Relates to #664 .
Description
This branch featues a draft for config validation and respective tests.
Checklist
docfolder updates etc.)