Combine parameters and sensitivities into a single endpoint #1397
Conversation
jorgenherje
left a comment
jorgenherje
left a comment
There was a problem hiding this comment.
Looks good. Some minor comments from my side :)
| sumo_client = create_sumo_client(access_token) | ||
| return cls(sumo_client=sumo_client, case_uuid=case_uuid, ensemble_name=ensemble_name) | ||
|
|
||
| async def get_parameters_and_sensitivities_async(self) -> EnsembleParameters: |
There was a problem hiding this comment.
Consider changing return type to be a tuple: (list[EnsembleParameter], list[EnsembleSensitivity]
|
|
||
| parameters: List[EnsembleParameter] | ||
| sensitivities: Optional[List[EnsembleSensitivity]] = None | ||
| sensitivities: List[EnsembleSensitivity] |
There was a problem hiding this comment.
I think we can remove the EnsembleParameters class and then just make the return type of ParameterAccess.get_parameters_and_sensitivities_async() to be tuple: (list[EnsembleParameter], list[EnsembleSensitivity]). If not we should rename the class to EnsembleParametersAndSensitivities
| @@ -1,2 +1,2 @@ | |||
| import logging | |||
| from typing import List, Optional, Literal | |||
|
|
||
| from primary.auth.auth_helper import AuthHelper | ||
| from webviz_services.sumo_access.parameter_access import ParameterAccess | ||
| from webviz_services.sumo_access.parameter_types import EnsembleParameter, EnsembleSensitivity |
There was a problem hiding this comment.
Unused imports (assume it will still be unused if you adjust according to previous comments)
| class EnsembleParameter(BaseModel): | ||
| """Description/data for a single parameter in an ensemble""" | ||
|
|
||
| name: str | ||
| is_logarithmic: bool | ||
| is_discrete: bool # values are string or integer | ||
| is_constant: bool # all values are equal | ||
| group_name: Optional[str] = None | ||
| descriptive_name: Optional[str] = None | ||
| is_discrete: bool | ||
| realizations: List[int] | ||
| values: Union[List[float], List[int], List[str]] |
There was a problem hiding this comment.
I know it hasn't been added now, but should we have camelCase naming in this file?
| const parametersAndSensitivitiesOutcome = parametersAndSensitivitiesOutcomeArray[i]; | ||
| console.debug(`parametersAndSensitivitiesOutcome[${i}]:`, parametersAndSensitivitiesOutcome.status); | ||
| let parameterArray: EnsembleParameter_api[] = []; | ||
| if (parametersOutcome.status === "fulfilled") { | ||
| parameterArray = parametersOutcome.value; | ||
| let sensitivityArray: EnsembleSensitivity_api[] = []; | ||
| if (parametersAndSensitivitiesOutcome.status === "fulfilled") { | ||
| parameterArray = parametersAndSensitivitiesOutcome.value.parameters; | ||
| sensitivityArray = parametersAndSensitivitiesOutcome.value.sensitivities; | ||
| } else { | ||
| const errorMessage = "Error fetching ensemble parameters, dropping ensemble."; | ||
| console.error(errorMessage, ensembleIdentString); | ||
| ensembleLoadingErrorInfoMap[ensembleIdentString] = { | ||
| errorMessage: errorMessage, | ||
| displayName: createRegularEnsembleDisplayName(ensembleIdents[i]), | ||
| }; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I know we did not check it previously, because the back-end failed if there was no parameters. But should we have a check for parameterArray.length === 0 to ensure that the ensemble has non-empty parameter array?
2 participants
Closes #1396