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

[SiApp] p-norm damage resp #12373

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[SiApp] p-norm damage resp #12373

wants to merge 8 commits into from

Conversation

Igarizza
Copy link
Member

📝 Description
PR implments a new aggregation strategy for damage detection resonse. Implementation creates new cpp object for p-norm calculations. In json file one can specify the sensor settings to use p-norm for weighting the sensor contribution.

Please mark the PR with appropriate tags:

  • Api Breaker, Mpi, etc...

🆕 Changelog
Please summarize the changes in one list to generate the changelog:
E.g.

  • Added feature X to Y
  • Added Foo Application
  • Fixed X (#XXXX Reference to issue if apply)
  • etc...

@Igarizza Igarizza requested a review from sunethwarna May 13, 2024 11:03
Copy link
Member

@sunethwarna sunethwarna left a comment

Choose a reason for hiding this comment

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

Thanks @Igarizza. Following are my comments:

  1. I know we don't have any small finite diferencing unit tests for the MeasurementResidualResponseFunction, but at least we can try adding a small finite differencing unit tests for the MeasurementResidualPNorm.

  2. Just an after thought, If we say p_coefficient = 1, then it is the existing MeasurementResidualPNorm ryt (apart from the 1/p at the end? should we unify these two?

Apart from above, I only have some minor comments.

Copy link
Member

@sunethwarna sunethwarna left a comment

Choose a reason for hiding this comment

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

Minor comments

@@ -36,7 +37,11 @@ def Initialize(self):
model_part.ProcessInfo[KratosSI.ADAPT_PERTURBATION_SIZE] = sensor_settings["adapt_perturbation_size"].GetBool()
self.listof_sensors = GetSensors(model_part, sensor_settings["list_of_sensors"].values())

self.measurement_residual_response_function = KratosSI.Sensors.MeasurementResidualResponseFunction()
aggregation_technique = sensor_settings["aggregation_technique"].GetString()
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 see how this aggregation_technique variable is used?

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 is left over, I will remove

…_custom_sensors_to_python.cpp

Co-authored-by: Suneth Warnakulasuriya <suneth.warna@gmail.com>
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.

None yet

2 participants