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 the ability to have arithmetic operators for responses #12580

Merged
merged 39 commits into from
Aug 5, 2024

Conversation

sunethwarna
Copy link
Member

@sunethwarna sunethwarna commented Jul 28, 2024

📝 Description
This PR adds the ability for the user to provide responses with arithmetic operators. Assume user has defined "r1" which is the mass response function, and "r2" as the linear strain energy reponse function. Then following can be used as an objective or constraint (an response function expression).

r1 + r2 * r1 + 4 * r1 ^ r2

Linking this response function expressions with algorithms will be done in a followup PR (see #12582).

Followings are implemented.

  • Brackets operator
  • "^" : Power operator
  • "/" : Division operator
  • "*" : Multiplication operator
  • "+" : Addition operator
  • "-" : Substract operator
  • log: Natural logarithmic function

🆕 Changelog

  • Brackets operator
  • "^" : Power operator
  • "/" : Division operator
  • "*" : Multiplication operator
  • "+" : Addition operator
  • "-" : Substract operator
  • log: Natural logarithmic function
  • Added tests

Requires #12579

FYI: @talhah-ansari

https://github.com/orgs/KratosMultiphysics/projects/7/views/1?pane=issue&itemId=71545497

@sunethwarna sunethwarna requested a review from matekelemen July 30, 2024 05:01
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 Thank you for your work. In general I like the new feature, it looks very well. I don't like the concept of EvaluateValue/Gradients. I think it is duplication of the existing Reponse Routine in some sense. If we need we can move some functionality to Reponse Function that it can decide if smth should be computed or return from memory.

BinaryOperatorValuesMap = dict([(operator.value, operator) for operator in BinaryOperator])

def GetClosingBracketIndex(expression: str) -> int:
# this function assumes the starting point is without the opening bracket.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# this function assumes the starting point is without the opening bracket.
# this function assumes the starting point is without the closing bracket.

Copy link
Member Author

Choose a reason for hiding this comment

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

it assume without the opening bracket.

Copy link
Member

Choose a reason for hiding this comment

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

The function name says you return the closing bracket. So, I think you assume there is no closing bracket at first position because I think we can assume that there is a opening bracket in the first position like "(a + b)*2"

raise RuntimeError(f"The closing bracket not found for expression ({expression}")

def GetFunction(function_name: str, optimization_problem: OptimizationProblem, *args) -> ResponseFunction:
# import functions
Copy link
Member

Choose a reason for hiding this comment

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

left over

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm.... It is not, i cannot import it at the top due to cyclic dependency.

Copy link
Member

Choose a reason for hiding this comment

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

but it is commented ?

return self.model_part

def CalculateValue(self) -> float:
v1 = EvaluateValue(self.response_function_1, self.optimization_problem)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the concept of evaluate. Why can't we just use Response Routine and it deifnes if smth should be computed or return from value directly ? It looks like dublication or bad design of response / response routine .

Copy link
Member Author

@sunethwarna sunethwarna Aug 4, 2024

Choose a reason for hiding this comment

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

Assume the scenario where "r1 + r2 * r1", where r1 needs to be evaluated twice. Instead of evaluating them twice, I use the EvaluteValue method to store the first evaluation of r1, and then call it in the second evaluation. I agree to your point that, instead of saving it in the OptProb, under respones, I will use a different place to save it. So, saving the value under the OptProb is decided by the response routine, and the call to CalculateValue will be normal per one evaluation. That means, for one call to CalculateValue/CalculateGradients of "r1 + r2 * r1", it will only evaluate r1 once, but for multiple CalculateValue/CalculateGradients, r1 and r2 will be evaluated once per CalculateValue/CalculateGradients calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same for the gradients

@sunethwarna
Copy link
Member Author

@Igarizza I addressed your comments, and changed the behaviour of the CalculateValue. Now, when ever CalculateValue is called, it does not rely on (theoratically) the memory, unless for same expression needs t be evaluated twice. Hence, this luxury is kept within the ResponseRoutine.

@sunethwarna sunethwarna requested a review from Igarizza August 4, 2024 09:07
@sunethwarna sunethwarna enabled auto-merge August 5, 2024 05:27
def EvaluateValue(response_function: ResponseFunction, optimization_problem: OptimizationProblem) -> float:
response_data = ComponentDataView("evaluated_responses", optimization_problem).GetUnBufferedData()

if not response_data.HasValue(f"values/{response_function.GetName()}"):
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if it is safe to check only in the data because we usually check based on the given x ...

from KratosMultiphysics.OptimizationApplication.utilities.response_utilities import BinaryOperator

class BinaryOperatorResponseFunction(ResponseFunction):
def __init__(self, response_function_1: ResponseFunction, response_function_2: ResponseFunction, binary_operator: BinaryOperator, optimization_problem: OptimizationProblem):
Copy link
Member

Choose a reason for hiding this comment

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

add class describtion please

from KratosMultiphysics.OptimizationApplication.utilities.buffered_dict import BufferedDict

class EvaluationResponseFunction(ResponseFunction):
def __init__(self, response_function: ResponseFunction, optimization_problem: OptimizationProblem):
Copy link
Member

Choose a reason for hiding this comment

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

add class description please

unbuffered_data = ComponentDataView("evaluated_responses", self.optimization_problem).GetUnBufferedData()

# reset data of the evaluation
self.__ResetEvaluationData(self, unbuffered_data, "values")
Copy link
Member

Choose a reason for hiding this comment

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

this is good

@sunethwarna sunethwarna merged commit 2b11fc6 into master Aug 5, 2024
9 of 11 checks passed
@sunethwarna sunethwarna deleted the optapp/responses/arithmetic_operators branch August 5, 2024 15:01
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.

Great job @suneth!

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.

2 participants