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

[CoSimulationApplication] Adding VTK debugging option for Kratos Mapping #12511

Merged
merged 28 commits into from
Oct 9, 2024

Conversation

loumalouomega
Copy link
Member

@loumalouomega loumalouomega commented Jul 5, 2024

📝 Description

Adding new mapping class with VTK debuggin, derived from Kratos Mapping.

For example can be sued as follows:

"data_transfer_operators" : {
      "mapper" : {
          "type" : "kratos_mapping_with_debug",
          "mapper_settings" : {
              "mapper_type" : "nearest_neighbor"
          },
          "debug_settings" : {
              "auxiliary_debug_counter" : true
          }
      }
  }

It will generate VTK folder for the pre and post mapping.

🆕 Changelog

@loumalouomega
Copy link
Member Author

For some reason FSI tests are failing in the FSIApp but this does not affect that application.

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

Will do a review on the weekend

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

I think this is a good addition, but the implementation should be moved to a CouplingOperation instead, this is the intended place for such things

Check for example the CouplingOutput which has a similar purpose

=> then the user can add it to the coupling_operations and call it before_data_transfer_operations or after_data_transfer_operations

pre_origin_settings["Parameters"]["output_path"].SetString(origin_output_path + "_pre_map")

# Create a VTK output process object with the provided settings and the current model
pre_process_origin = vtk_output_process.Factory(pre_origin_settings, model_origin)
Copy link
Member

Choose a reason for hiding this comment

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

I would use the vtk_output directly, then you only need to call PrintOutput, and not have all the overhead of the process

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand your point, but the problem with that impliementation is that I will need to manually add the settings for the IO instead of automatically deduce the variables and settings from the mapping like I do in the current proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

@loumalouomega
Copy link
Member Author

We should discuss this @philbucher. Currently I need to transfer the files to the interested people for debug and it is a bit inconvinient

The commit refactors the `KratosMappingDataTransferOperator` class in the `CoSimulationApplication` to improve VTK debugging. It adds an auxiliary counter for wrappers without `TIME` or `STEP` defined, and updates the output paths for VTK output processing. The changes ensure that the VTK output files are saved in the correct directory and have appropriate names.
@philbucher
Copy link
Member

oh sorry I completely forgot, will take a look over the weekend
Pls let me know when you are done with your changes

@loumalouomega
Copy link
Member Author

A priori it is done, I am just adding some minor fixes from internal feedback

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

I had a look and TBH its very complicated and not very nicely done

How about you derive a class from it, e.g. kratos_mapping_with_debug_output to not mess up the baseclass?

Copy link
Member

Choose a reason for hiding this comment

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

please move the additions in this file, they should not be in the baseclass

@loumalouomega
Copy link
Member Author

I had a look and TBH its very complicated and not very nicely done

Theare are better ways to express that :P

How about you derive a class from it, e.g. kratos_mapping_with_debug_output to not mess up the baseclass?

I could do that

Co-authored-by: Philipp Bucher <[email protected]>
@philbucher
Copy link
Member

Theare are better ways to express that :P

I tried my best, sry 😓

@loumalouomega
Copy link
Member Author

Theare are better ways to express that :P

I tried my best, sry 😓

Should be Okay now

@loumalouomega
Copy link
Member Author

Suggestions applied @philbucher

@loumalouomega
Copy link
Member Author

Are you Okay now @philbucher ¿?

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

👍

from KratosMultiphysics.CoSimulationApplication.utilities import data_communicator_utilities
from time import time
from dataclasses import dataclass
from typing import Union, Tuple
Copy link
Member

Choose a reason for hiding this comment

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

ah my archnemesis, typing in python ...

/rant 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.

It was your suggestion...

Copy link
Member

Choose a reason for hiding this comment

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

I know, its the direction the language goes, smarter ppl than me design this

still, I can complain about it :D

@loumalouomega loumalouomega merged commit 279f0a6 into master Oct 9, 2024
9 of 11 checks passed
@loumalouomega loumalouomega deleted the CoSim/adding-vtk-debugging-kratos-mapping branch October 9, 2024 11:16
@loumalouomega
Copy link
Member Author

@roigcarlo do you think it is possible to cherry-pick this into a minor release?

@loumalouomega
Copy link
Member Author

Finally

imagen

@philbucher
Copy link
Member

Finally

imagen

still does not take as long as my phd ...

@roigcarlo
Copy link
Member

I Will include it in 10.0

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