feat(reserves): support thermal reserves#3280
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
sylvlecl
left a comment
sylvlecl
left a comment
There was a problem hiding this comment.
I think at least a few points need to be fixed (possibility to remove certifications and symmetries of a reserve), and other points need to be clarified (behaviour when we delete a reserve, a cluster, and handling of missing reserves in dicts).
| return ThermalReserveCertification.model_validate(self.model_dump(exclude={"reserve"})) | ||
|
|
||
|
|
||
| def serialize_thermal_reserve_certifications( |
There was a problem hiding this comment.
would it not be more clear to have a larger "file data" model consistent with the scope of the file (certifications + symmetries), and return that one, and use the same one when updating symmetries ?
| commands: list[ICommand] = [] | ||
|
|
||
| # Thermals part | ||
| if data.thermals: |
There was a problem hiding this comment.
if the list is empty, we want to replace by an empty list, otherwise we will never be able to remove all certifications
| commands: list[ICommand] = [] | ||
|
|
||
| # Thermal part | ||
| if data.thermals: |
There was a problem hiding this comment.
Same here: if empty we want to replace with an empty list
| study_data.save_load({area_id: null_matrix}) | ||
| study_data.save_solar({area_id: null_matrix}) | ||
| study_data.save_wind({area_id: null_matrix}) | ||
| study_data.save_misc_gen({area_id: constants.get_default_miscgen()}) |
| from antarest.study.dao.common import AreaId | ||
|
|
||
|
|
||
| class ReadOnlyThermalReserveCertificationDao(ABC): |
There was a problem hiding this comment.
honestly we could have kept only one file for the whole reserves stuff, I think :)
We will end up with quite a lot of files with the other stuff (hydro and STS)
| Column("area_id", String(255), nullable=False, primary_key=True), | ||
| Column("thermal_id", String(255), nullable=False, primary_key=True), | ||
| Column("symmetries", String(), nullable=False), | ||
| ForeignKeyConstraint( |
There was a problem hiding this comment.
what happens when we delete a reserve ?
It seems to me that we should delete them in symmetries too.
To implement it, maybe we should a different DB structure ?
However, maybe we also want to remove symmetries that only contain 1 item after change ...
Don't know if it could cause an error on the simulator side.
| return self._adaptee.get_all_thermal_reserve_certifications() | ||
|
|
||
| @override | ||
| def get_thermal_reserve_certifications( |
There was a problem hiding this comment.
We should mention what's expected when there is no certification for an existing reserve:
is the reserve absent from the mapping, or do we expect to have all existing reserves instead, with possibly empty dicts? (the second option is more clear from the interface point of view).
Same in the save method actually: do we allow to have absent reserves ? In that case does that mean that we must empty the list for that reserve ? we need to explain it in the docstring
There was a problem hiding this comment.
Seems to me that if a reserve is absent, we will not update it, in the DB implementation at least.
Same for the get.
It can be a little misleading, because it now looks partially like an update.
Maybe it's simpler to leave it this way but we need to document it in the docstring and probably in the API doc too
| from antarest.study.storage.variantstudy.model.model import CommandDTO | ||
|
|
||
|
|
||
| class ReplaceThermalReserveCertifications(ICommand): |
There was a problem hiding this comment.
Same here, if we keep the behaviour we should document it: we could think that we will replace all the certifications of the area, whereas we will only replace the ones of reserves that are present in the mapping
2 participants
4 new endpoints introduced:
/studies/{uuid}/areas/{area_id}/reserves/symmetries/studies/{uuid}/areas/{area_id}/reserves/symmetries/studies/{uuid}/areas/{area_id}/reserves/certifications/studies/{uuid}/areas/{area_id}/reserves/certifications