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

[Core] Adding deprecation message to CheckSameModelPartUsingSkinDistanceProcess + check dimension #10715

Merged
merged 12 commits into from
Oct 27, 2023

Conversation

loumalouomega
Copy link
Member

@loumalouomega loumalouomega commented Feb 2, 2023

📝 Description

Adding deprecation message to CheckSameModelPartUsingSkinDistanceProcess + check dimension. Also adding pure model part constructor as well.

After comment from #10704

🆕 Changelog

Co-authored-by: Philipp Bucher <philipp.bucher@tum.de>
@philbucher philbucher removed their request for review February 3, 2023 20:23
KratosMultiphysics.Process.__init__(self)

# Assigning values
self.model = model

# Get first model part (must be defined anyway)
self.model_part = self.model[settings["skin_model_part_1_name"].GetString()]
self.dimension = self.model_part.ProcessInfo[KratosMultiphysics.DOMAIN_SIZE]
Copy link
Member

Choose a reason for hiding this comment

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

I'd check that the ProcessInfo has the domain size to throw a meaningful error if not provided.

Comment on lines 88 to 95
explicit CheckSameModelPartUsingSkinDistanceProcess(
ModelPart& rSkinModelPart1,
ModelPart& rSkinModelPart2,
Parameters ThisParameters = Parameters(R"({})")
)
: mrSkinModelPart1(rSkinModelPart1),
mrSkinModelPart2(rSkinModelPart2),
mThisParameters(ThisParameters)
Copy link
Member

Choose a reason for hiding this comment

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

Why adding a pure model part constructor? The line of the project is to promote the model-based ones. If the reasoning behind is to use more than one model, in my opinion giving this possibility is not a good idea. Note that the line of the project is to have a unique Model container, also in the multistage case.

Copy link
Member Author

Choose a reason for hiding this comment

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

(yes, that's the idea, @pooyan-dadvand wants to use it to check it in CoSim and the same model is not guaranteed)

Copy link
Member

Choose a reason for hiding this comment

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

Though this is a fair usage, I'm not sure if we want to open this possibility in the entire Kratos core. To be discussed in @KratosMultiphysics/technical-committee.

Copy link
Member

@rubenzorrilla rubenzorrilla left a comment

Choose a reason for hiding this comment

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

Giving support to "multi-model" simulations is out of the scope of this PR and must be discussed by the @KratosMultiphysics/technical-committee.

@loumalouomega
Copy link
Member Author

Giving support to "multi-model" simulations is out of the scope of this PR and must be discussed by the @KratosMultiphysics/technical-committee.

  1. I commented the code
  2. There are many processes with modelpart in the constructor instead of Models (specially the old ones)

@rubenzorrilla
Copy link
Member

2. There are many processes with modelpart in the constructor instead of Models (specially the old ones)

Exactly, these come from the pre-model era. New ones must use the model. This is why I said "The line of the project is to promote the model-based ones".

@rubenzorrilla rubenzorrilla added this to To do in LEGACY Technical Committee via automation Feb 14, 2023
@rubenzorrilla rubenzorrilla moved this from To do to Next meeting todo in LEGACY Technical Committee Feb 14, 2023
@rubenzorrilla
Copy link
Member

@KratosMultiphysics/technical-committee has been discussing about the points raised by @rubenzorrilla.

  1. About the "multi-model" support. Even though in the context of CoSimulation this might be a fair usage, we would stick to current design guidelines and have a unique Model container. If in the future this becomes in a technical limitation we will reopen the "multi-model" discussion again.

  2. About the multiple constructors. The @KratosMultiphysics/technical-committee agrees on promoting Model-parameters ones because this eases a lot code maintenance and allows standarized factories. However, there might be cases in which other constructors might handy or even required (e.g. passing a linear solver). Hence, we wouldn't ban the addition of new constructors provided a technical justification.

@rubenzorrilla rubenzorrilla moved this from Next meeting todo to In progress in LEGACY Technical Committee Feb 24, 2023
@loumalouomega
Copy link
Member Author

@rubenzorrilla I already removed (commented) the two modelpart constructor. In that case, this is ready. Anyway, the problem will be the CoSimulation design itself.

@philbucher
Copy link
Member

Anyway, the problem will be the CoSimulation design itself.

for the record, Aditya and I raised those concerns VERY LOUDLY during the implementation of the "single model" strategy

IMO this is a limitation of the "single model" design

@loumalouomega
Copy link
Member Author

Anyway, the problem will be the CoSimulation design itself.

for the record, Aditya and I raised those concerns VERY LOUDLY during the implementation of the "single model" strategy

IMO this is a limitation of the "single model" design

I wasn't in that discussion, I just realized that design limitation as soon as I started with the implementation...

@loumalouomega
Copy link
Member Author

I forgot what this about...

@loumalouomega
Copy link
Member Author

I forgot what this about...

@rubenzorrilla do you remember?

@loumalouomega
Copy link
Member Author

I forgot what this about...

@rubenzorrilla do you remember?

Hellooooooooooooooooooooooo

Co-authored-by: Philipp Bucher <philipp.bucher@tum.de>
loumalouomega and others added 3 commits October 27, 2023 10:05
Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>
Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>
Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>
@loumalouomega loumalouomega merged commit c914524 into master Oct 27, 2023
9 of 11 checks passed
LEGACY Technical Committee automation moved this from In progress to Done Oct 27, 2023
@loumalouomega loumalouomega deleted the core/adding-warning-process branch October 27, 2023 14:07
@loumalouomega
Copy link
Member Author

At last!, I don't even remember what was this PR doing :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecated Kratos Core Python Refactor When code is moved or rewrote keeping the same behavior Warning
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants