Conversation
src/gems/simulation/output_values.py
Outdated
| _name: str | ||
| _value: Dict[TimeScenarioIndex, float] = field(init=False, default_factory=dict) | ||
| _size: Tuple[int, int] = field(init=False, default=(0, 0)) | ||
| _basis_status: Optional[str] = field(default=None, init=False) # NEW Ali |
src/gems/simulation/output_values.py
Outdated
| return string | ||
|
|
||
| def _build_components(self) -> None: | ||
| def _build_components(self) -> None: # old build |
src/gems/simulation/output_values.py
Outdated
| _basis_status: Optional[str] = field(default=None, init=False) # NEW Ali | ||
| ignore: bool = field(default=False, init=False) | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
You do not need an __init__ method, it is generated by the dataclass, using the information given in the field(default=..., init= ...)
There was a problem hiding this comment.
The only difference I see with the previous behaviour is that you want to set _value to value if is given as argument, however this is forbidden as it has init=False so your init method basically is the one automatically generated
src/gems/simulation/output_values.py
Outdated
| if self.problem.solver.IsMip(): | ||
| return | ||
| for key, value in self.problem.context.get_all_component_variables().items(): | ||
| var_obj = self.component(key.component_id).var(str(key.variable_name)) |
There was a problem hiding this comment.
Why not just write self.component(key.component_id).var(str(key.variable_name)) = value.basis_status() inside the for loop in _build_components ?
- This avoids going through the dict of component variables a second time (one in
build_components, one in_set_basisin your case) - The logic is that
build_componentsbuild the whole component, thereofore setting all properties of the variables, including the basis_status. Therefore it is weird to split the basis setting part from the solution value setting - Avoid copying the variable object, just use a one liner to set the basis status
There was a problem hiding this comment.
Obviously, move the mip check in consequence
| simulation_id: str | ||
| df: pd.DataFrame | ||
|
|
||
| def __init__(self, simulation_id: Optional[str] = None, mode: str = "eco") -> None: |
| self.df = pd.DataFrame(rows) | ||
|
|
||
| # Ensure problem is set before accessing solver | ||
| if output_values.problem is None: |
There was a problem hiding this comment.
This check should be done at the very beginning of the build (you need the problem to access the block length, scenarios, ...)
And it is best practice to raise errors as soon as possible, to avoid losing time
| "value": objective_value, | ||
| "basis_status": None, | ||
| } | ||
| self.df.loc[len(self.df)] = [obj_row.get(col, None) for col in self.df.columns] |
There was a problem hiding this comment.
This does not not work ?
| self.df.loc[len(self.df)] = [obj_row.get(col, None) for col in self.df.columns] | |
| self.df.loc[len(self.df)] = obj_row |
| @@ -0,0 +1,69 @@ | |||
| # Standard library imports | |||
| from dataclasses import dataclass | |||
| from pathlib import Path | ||
|
|
||
| # Third-party imports | ||
| import pandas as pd |
|
|
||
| # --- Assertions --- | ||
| assert csv_path.exists(), "Simulation table CSV not created" | ||
| assert not simu_table.df.empty, "Simulation table dataframe is empty" |
There was a problem hiding this comment.
Ok for now, but we may think of a more robust test
- When we split the class with the builder and writer, the test will be on the content of the dataframe (maybe check its length, its values if possible (may be hard))
| timestep = 0 if timestep is None else timestep | ||
| scenario = 0 if scenario is None else scenario | ||
| key = TimeScenarioIndex(timestep, scenario) | ||
| if key not in self._value: |
There was a problem hiding this comment.
I think this has nothing to do here, this is done by the set method. Maybe the simplest is to add self._basis_status[key] = status to the _set method, in the logic that _set is responsible for setting all relevant attribute of the variable (then the isMip check should also be in _set)
| BLOCK_TIME_INDEX = "block-time-index" | ||
| SCENARIO_INDEX = "scenario-index" | ||
| VALUE = "value" | ||
| BASIS_STATUS = "basis-status" |
There was a problem hiding this comment.
The convention is to use _ rather than - in column names
There was a problem hiding this comment.
it is writne "in kebab case" in the specs. of the Modeleur. I thought It is wise to keep consistent with it. Up to you
| context = output_values.problem.context | ||
| block = context._block.id | ||
| block_size = context.block_length() | ||
| absolute_time_offset = (block - 1) * block_size |
There was a problem hiding this comment.
This assumes that all blocks are the same size... this may not be the case in the most general case
There was a problem hiding this comment.
This is why I think this is ok to have this as argument
| + ts_index.time, | ||
| SimulationColumns.BLOCK_TIME_INDEX.value: ts_index.time, | ||
| SimulationColumns.SCENARIO_INDEX.value: ts_index.scenario, | ||
| SimulationColumns.VALUE.value: value, |
There was a problem hiding this comment.
Why do you need .value everywhere ?
| class SimulationTableWriter: | ||
| """Handles writing simulation tables to CSV.""" | ||
|
|
||
| def __init__(self, simulation_table: pd.DataFrame) -> None: |
There was a problem hiding this comment.
You could use a dataclass
2 participants
simulation table added,
output value extended to make simultation table based only on output values. Except for objective value (why not)
Also, tests have been updated