-
Notifications
You must be signed in to change notification settings - Fork 63
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
Introduce Named Data Classes for mlos_core #811
Conversation
elif left is not None and right is not None: | ||
if not left.equals(right): | ||
return False | ||
return True |
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 can be simplified a bit:
if left is not None and right is not None and left.equals(right):
return True
return False
def __repr__(self) -> str: | ||
return ( | ||
f"Observation(config={self.config}, score={self.score}," | ||
+ " context={self.context}, metadata={self.metadata})" |
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.
same concat comment here
scores: pd.DataFrame, | ||
context: Optional[pd.DataFrame] = None, | ||
metadata: Optional[pd.DataFrame] = None, | ||
observation: Observation, |
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.
observation: Observation, | |
observations: Observations, |
@@ -150,7 +134,7 @@ def suggest( | |||
*, | |||
context: Optional[pd.DataFrame] = None, |
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.
context: Optional[pd.DataFrame] = None, | |
context: Optional[pd.Series] = None, |
Did this one get replacd by a Series elsewhere?
""" | ||
pass # pylint: disable=unnecessary-pass # pragma: no cover | ||
|
||
def get_observations(self) -> Tuple[pd.DataFrame, pd.DataFrame, Optional[pd.DataFrame]]: | ||
def get_observations(self) -> Observation: |
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.
def get_observations(self) -> Observation: | |
def get_observations(self) -> Observations: |
docstring needs updating too
).reset_index(drop=True) | ||
return (configs, scores, contexts if len(contexts.columns) > 0 else None) | ||
return Observation( |
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.
return Observation( | |
return Observations( |
return Observation( | ||
config=configs, | ||
score=scores, | ||
context=contexts if len(contexts.columns) > 0 else None, |
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.
context=contexts if len(contexts.columns) > 0 else None, | |
context=contexts if len(contexts) > 0 else None, |
?
|
||
def get_best_observations( | ||
self, | ||
*, | ||
n_max: int = 1, | ||
) -> Tuple[pd.DataFrame, pd.DataFrame, Optional[pd.DataFrame]]: | ||
) -> Observation: |
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.
) -> Observation: | |
) -> Observations: |
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.
docstring again
return (configs.loc[idx], scores.loc[idx], None if contexts is None else contexts.loc[idx]) | ||
observations = self.get_observations() | ||
idx = observations.score.nsmallest( | ||
n_max, columns=self._optimization_targets, keep="first" |
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.
n_max, columns=self._optimization_targets, keep="first" | |
n_max, columns=self._optimization_targets, keep="first", |
trailing comma, format fixups
|
||
|
||
def compare_optional_dataframe( | ||
left: Optional[pd.DataFrame], right: Optional[pd.DataFrame] |
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.
left: Optional[pd.DataFrame], right: Optional[pd.DataFrame] | |
left: Optional[pd.DataFrame], right: Optional[pd.DataFrame], |
nit: trailing comma and black reformat
Got confused and started commenting here again. I think #852 supercedes this one, so let's close this one. |
Introduces
@dataclass
types for return values from mlos_core.Optimizers to improve code readability.See also: #751