refactor: use time series collection yaml classes #328
Conversation
| @@ -269,8 +273,15 @@ def facility_inputs(self): | |||
| return self._internal_datamodel.get(EcalcYamlKeywords.facility_inputs, []) | |||
|
|
|||
| @property | |||
| def time_series(self): | |||
| return self._internal_datamodel.get(EcalcYamlKeywords.time_series, []) | |||
| def time_series(self) -> List[YamlTimeSeriesCollection]: | |||
There was a problem hiding this comment.
Should do this for all values in yaml model IMO. Doing it here when the properties are used makes it possible to work with the yaml file without everything being valid
There was a problem hiding this comment.
so, to only actually parse accordign to schema when we use a property?
There was a problem hiding this comment.
yes, we can have a separate validate function if we need to make sure the full model is valid.
There was a problem hiding this comment.
We could also require the model to be valid.
It would be nice if this class took care of it though. This class would then read the model, then we can get the properties in the model as yaml classes, removing the need to validate a lot of stuff in other places.
| @@ -11,6 +13,7 @@ | |||
| class YamlTimeSeriesCollectionBase(YamlBase): | |||
| class Config: | |||
| title = "TimeSeries" | |||
| use_enum_values = True | |||
There was a problem hiding this comment.
Makes enum a str when calling dict()
| @@ -112,7 +112,8 @@ def map_yaml_to_variables( | |||
| result_options: dto.ResultOptions, | |||
| ) -> dto.VariablesMap: | |||
| timeseries_collections = [ | |||
| TimeSeriesCollectionMapper(resources).from_yaml_to_dto(timeseries) for timeseries in configuration.time_series | |||
| TimeSeriesCollectionMapper(resources).from_yaml_to_dto(timeseries.dict(by_alias=True)) | |||
There was a problem hiding this comment.
We should look into removing this mapper and the classes, should be able to handle it in the yaml classes?
Converting back to dict as that is expected in TimeSeriesCollectionMapper
There was a problem hiding this comment.
you mean to implicitly and automatically handle the conversion?
There was a problem hiding this comment.
https://equinor-ecalc.atlassian.net/browse/ECALC-586 Explained a bit here.
TimeSeriesCollectionMapper should not need to deal with validation of the yaml at least, it could be simplified to only deal with reading the resources and creating something that can easily be added to variables map
Validate when getting time_series from yaml model.
jsolaas
force-pushed
the
refactor/use-time-series-yaml-classes
branch
from
aceffca
to
93baf08
Compare
| @@ -269,8 +273,15 @@ def facility_inputs(self): | |||
| return self._internal_datamodel.get(EcalcYamlKeywords.facility_inputs, []) | |||
|
|
|||
| @property | |||
| def time_series(self): | |||
| return self._internal_datamodel.get(EcalcYamlKeywords.time_series, []) | |||
There was a problem hiding this comment.
what was returned here? a "string dict"?
There was a problem hiding this comment.
Previously it was a list of dicts, each dict was a time series entry from yaml
Validate when getting time_series from yaml model.