Skip to content
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

[OptApp] Adding combined response function #12432

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

sunethwarna
Copy link
Member

📝 Description
This PR adds CombinedResponseFunction which can be used to combine responses in a recursive manner.

🆕 Changelog

  • Added CombinedResponseFunction
  • Added tests

@matekelemen
Copy link
Contributor

I think it'd make sense if you gave these response functions the same treatment as expressions. I.e.: overload the operators of ResponseFunction to yield a compound one.

@sunethwarna
Copy link
Member Author

sunethwarna commented Jun 9, 2024

I think that is an overkill for compound responses. Even for averaging out multiple responses, we need to be very careful about the weighting. Another reason is, having an output mechanism for elements of the compound response will be much difficult

Copy link
Member

@Igarizza Igarizza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunethwarna thanks for your work. In general, it does look great! I have only one worry, I have placed in the code.

response_params.ValidateAndAssignDefaults(default_settings["combining_responses"].values()[0])
self.list_of_responses.append(
(
self.optimization_problem.GetResponse(response_params["response_name"].GetString()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you make sure that this response is called after other ones are already added to the optimization_problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot. Responses should be added in the order which they are used. It is the users responsibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, add a descriptive error so that the user can understand why it doesn't work (if the response is not found in the optimization_problem object)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

Igarizza
Igarizza previously approved these changes Jun 12, 2024
response_params.ValidateAndAssignDefaults(default_settings["combining_responses"].values()[0])
self.list_of_responses.append(
(
self.optimization_problem.GetResponse(response_params["response_name"].GetString()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, add a descriptive error so that the user can understand why it doesn't work (if the response is not found in the optimization_problem object)

@sunethwarna
Copy link
Member Author

@Igarizza I have added a descriptive error. Could you check?

@sunethwarna sunethwarna requested a review from Igarizza June 12, 2024 10:46
@sunethwarna sunethwarna enabled auto-merge June 12, 2024 13:04
@sunethwarna sunethwarna merged commit 3aadc26 into master Jun 12, 2024
11 checks passed
@sunethwarna sunethwarna deleted the optapp/responses/combined_response_function branch June 12, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants