-
Notifications
You must be signed in to change notification settings - Fork 110
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
Refactor and update for ropt 0.11 #9923
Conversation
4a4504a
to
d3738ed
Compare
CodSpeed Performance ReportMerging #9923 will not alter performanceComparing Summary
|
src/everest/everest_storage.py
Outdated
), | ||
} | ||
) | ||
|
||
def _store_function_results(self, results: FunctionResults) -> _EvaluationResults: | ||
names = { |
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.
(this comment is also scoped to the same thing in _store_gradient_results
) This ensures we select only stuff that is actually requested by Everest? Would ROPT ever generate names that are not in this list? Some kind of comment to clarify why we do this would be helpful, also own commit with descriptive comment.
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.
names
just collects the names of controls, objectives etc., so that the generated dataframe contains them, instead of an index. It might be possible/better to add the names to the dataframe afterwards, by replacing indices with names. Then we don't have to depend on ropt
for that. Do you think that would be doable?
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'm not fully sure I understand this, so having the names in the dataframe instead of an index? So adding them kind of does the same as we now do with the .reset_index()
? How would we know where to add which names and so on?
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 am doing it right now in an extra commit, so if that works I will push it and you will probably understand.
d3738ed
to
af07682
Compare
af07682
to
d3fda4e
Compare
src/everest/everest_storage.py
Outdated
@@ -592,6 +617,25 @@ def _store_gradient_results(self, results: GradientResults) -> _GradientResults: | |||
), | |||
] | |||
) | |||
batch_objective_gradient = batch_objective_gradient.with_columns( |
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 some kind of comment somewhere on why we need to do this would be good, it is not apparent that ropt operates only with the indices, and that we have to add the names back
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.
Added in the new _ropt_to_df
method.
d3fda4e
to
4cb4727
Compare
src/everest/everest_storage.py
Outdated
# control names, objective names, etc. The corresponding names can be | ||
# retrieved from the everest configuration and were stored in the init | ||
# method. Here we replace the indices with those names: | ||
replacements = [] |
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.
Could we shorten down a bit more to something like:
@dataclass
class _RoptToEverestCol:
ropt_column: str
everest_column: polars.Series
ropt_to_everest_cols = {
"variable": _RoptToEverestCol(
ropt_column="variable",
everest_column=self.data.controls["control_name"],
),
"objective": _RoptToEverestCol(
ropt_column="objective",
everest_column=self.data.objective_functions["objective_name"],
),
"nonlinear_constraint": _RoptToEverestCol(
ropt_column="nonlinear_constraint",
everest_column=self.data.objective_functions["objective_name"],
),
"realization": _RoptToEverestCol(
ropt_column="realization",
everest_column=self.data.nonlinear_constraints["constraint_name"],
),
}
for ropt_col, everest_col in ropt_to_everest_cols.items():
if ropt_col in select:
df = df.with_columns(
polars.col(ropt_col).replace_strict(dict(enumerate(everest_col)))
)
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.
done.
4cb4727
to
a45fce0
Compare
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.
Nice! LGTM
Issue
ropt
has been updated to version 0.11, with a few changes that affect Everest:ropt
does not store names of controls, objectives, constraints and realizations anymore, as it does not need themThese changes are intended to make
ropt
simpler and give more control to Everest over some aspects (such as control name handling). In addition it aims to make Everest less dependent onropt
internals.This PR does the following:
Implement changes needed to use
ropt==0.11
.Remove the output tables generated by
ropt
, since that should be done by Everest.Remove some unused functions in the everest data api
Remove some data from the everest storage that is not needed
PR title captures the intent of the changes, and is fitting for release notes.
Added appropriate release note label
Commit history is consistent and clean, in line with the contribution guidelines.
Make sure unit tests pass locally after every commit (
git rebase -i main --exec 'just rapid-tests'
)When applicable